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

Tue Apr 5 06:52:07 UTC 2016

Thanks you, Vladimir.

Regards,
Filipp.

On Mon, Apr 4, 2016 at 9:04 PM, Vladimir Kozlov
<vladimir.kozlov at oracle.com> wrote:
> On 4/4/16 8:16 AM, Mikael Gerdin 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*.
>> 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 haven't checked all the other uses but from a GC perspective I think
>> this is good to go.
>
>
> C1 changes are good too.
>
> Thanks,
> Vladimir
>
>
>>
>> /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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>
>