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

[Filipp Zhinkin]

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.

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

Also, it seems like in ParNewGeneration::collect we have to create
ResourceMark before ParScanThreadStateSet, right?

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