RFR (L): 8149374: Replace C1-specific collection classes with universal collection classes

Tue Apr 5 06:49:28 UTC 2016

Hi Mikael,

On Mon, Apr 4, 2016 at 6:16 PM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
> Hi Filipp,
>
> On 2016-04-02 13:32, Filipp Zhinkin wrote:
>>
>> Here is an webrev updated according Mikael's comments:
>>
>> http://cr.openjdk.java.net/~fzhinkin/8149374/webrev.02/
>
>
> I noticed that you changed this compared to my suggestion.
> From my understanding the explicit cast to void is unnecessary, all pointer
> types implicitly convert to void*.

Yes, it's unnecessary. I'll change.

> I've been forced to accept that people prefer "&foo[i]" over "foo + i" in
> the past so I'm ok with that change.
>
> -    new ((ParScanThreadState*)_data + i)
> +    new((void*) &_per_thread_states[i])
>
>
> I suspect that you can remove
>   33 // correct linkage required to compile w/o warnings
>   34 // (must be on file level - cannot be local)
>   35 extern "C" { typedef int (*ftype)(const void*, const void*); }
>   36
> as well, it appears to only have been used by the sort() methods of the
> removed array classes.

I'm too inattentive. :(

>
> I haven't checked all the other uses but from a GC perspective I think this
> is good to go.

Thanks!

Filipp.

>
> /Mikael
>
>
>>
>> Tested using hotspot_all tests w/ CMS turned on.
>>
>> Thanks,
>> Filipp.
>>
>> On Fri, Apr 1, 2016 at 5:34 PM, Mikael Gerdin <mikael.gerdin at oracle.com>
>> wrote:
>>>
>>> Hi Filipp
>>>
>>> On 2016-04-01 16:27, Filipp Zhinkin wrote:
>>>>
>>>>
>>>> Hi Mikael,
>>>>
>>>> On Thu, Mar 31, 2016 at 7:25 PM, Mikael Gerdin
>>>> <mikael.gerdin at oracle.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> I like the cleanup, can't we also remove CHeapArray in arrays.hpp?
>>>>
>>>>
>>>>
>>>> Sure! I've missed that it is not used at all.
>>>
>>>
>>>
>>> Great!
>>>
>>>>
>>>>>
>>>>> As for the CMS change, I would prefer this instead (untested!):
>>>>> http://cr.openjdk.java.net/~mgerdin/pss-array/webrev/
>>>>
>>>>
>>>>
>>>> Thanks, your implementation looks much better.
>>>> If you don't mind I'll incorporate it into my change.
>>>
>>>
>>>
>>> Go ahead, that was my intention.
>>>
>>>>
>>>> Also, it seems like in ParNewGeneration::collect we have to create
>>>> ResourceMark before ParScanThreadStateSet, right?
>>>
>>>
>>>
>>> There is a ResourceMark in the caller so I don't think it's needed.
>>> The old version of the code used resource allocation as well and was fine
>>> so
>>> I don't think there is a need to introduce another ResourceMark.
>>>
>>>
>>> /Mikael
>>>
>>>
>>>>
>>>> Thanks,
>>>> Filipp.
>>>>
>>>>>
>>>>> /Mikael
>>>>>
>>>>>
>>>>> On 2016-03-31 17:14, Vladimir Kozlov wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi Filipp,
>>>>>>
>>>>>> Yes, this looks better. CCing to hotspot-dev for Runtime and GC groups
>>>>>> to look on.
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 3/31/16 8:08 AM, Filipp Zhinkin wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hi Vladimir,
>>>>>>>
>>>>>>> thank you for looking at this change.
>>>>>>>
>>>>>>> On Wed, Mar 30, 2016 at 1:18 AM, Vladimir Kozlov
>>>>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Nice clean up but I don't see any source code removed. What benefits
>>>>>>>> we have
>>>>>>>> then?
>>>>>>>> I understand that we don't generate subclasses for ResourceArray and
>>>>>>>> use
>>>>>>>> GrowableArray. But it will not save space I think.
>>>>>>>> What prevents us to remove ResourceArray at all?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> CMS's ParScanThreadStateSet is inherited from ResourceArray,
>>>>>>> so it should be updated before removing ResourceArray:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~fzhinkin/8149374/webrev.01/
>>>>>>>
>>>>>>>>
>>>>>>>> On 3/11/16 3:42 AM, Filipp Zhinkin wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> please review a fix for JDK-8149374:
>>>>>>>>>
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~fzhinkin/8149374/webrev.00/
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8149374
>>>>>>>>> Testing done: hotspot_all tests + CTW
>>>>>>>>>
>>>>>>>>> I've replaced all usages of collections defined via define_array
>>>>>>>>> and
>>>>>>>>> define_stack macros with GrowableArray.
>>>>>>>>>
>>>>>>>>> There are good and bad news regarding performance impact of that
>>>>>>>>> change.
>>>>>>>>> Unfortunately, C1 compilation time for CTW-scenario w/ release bits
>>>>>>>>> increased from 51.07±0.28s to 52.99±0.23s (it's about 3.5%).
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> It is acceptable regression I think. I don't think we should
>>>>>>>> optimize
>>>>>>>> and
>>>>>>>> make more complex GrowableArray just to save 0.5% of performance for
>>>>>>>> C2.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> As long as GrowableArray is used in different Hotspot's subsystems it
>>>>>>> may be beneficial to optimize it,
>>>>>>> but I've executed SPECjvm2008's startup.* benchmarks and there were
>>>>>>> no
>>>>>>> significant difference.
>>>>>>>
>>>>>>> If ~3% regression is OK for C1 then I'm fine with leaving
>>>>>>> GrowableArray's initialization
>>>>>>> in its current state unless there will be other reasons to speed it
>>>>>>> up.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Filipp.
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Such difference caused by eager initialization of GrowableArray's
>>>>>>>>> backing array elements [1]. I can imagine when we actually need to
>>>>>>>>> force
>>>>>>>>> initialization and de-initialization during array's
>>>>>>>>> growing/destruction, but for some types like c++ primitive types or
>>>>>>>>> pointers such initialization does not make much sense, because
>>>>>>>>> GrowableArray is not allowing to access an element which was not
>>>>>>>>> explicitly placed inside of it. And as long as GrowableArray most
>>>>>>>>> widely used to store pointers we're simply wasting the time with
>>>>>>>>> initialization.
>>>>>>>>>
>>>>>>>>> I've measured CTW time with following workaround which implements
>>>>>>>>> initialization for numeric types and pointers as no-op and C1
>>>>>>>>> compilation time returned back to values that were measured before
>>>>>>>>> original change (51.06±0.24s):
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~fzhinkin/growableArrayInitialization/webrev/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I've also measured C2 compilation time and it dropped down by a few
>>>>>>>>> seconds too: 1138±9s w/o GrowableArray's change and 1132±5s w/ it.
>>>>>>>>>
>>>>>>>>> Summing up: I guess we should avoid GrowableArray's backing array
>>>>>>>>> initialization for some types, don't we?
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Filipp
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/323b8370b0f6/src/share/vm/utilities/growableArray.hpp#l165
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>
>>>
>