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

[Vladimir Kozlov]

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