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

Mon Apr 4 15:16:30 UTC 2016

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.

/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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>
>>