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

[Coleen Phillimore]

Thank you for CCing hotspot-dev.   This change is great!   I reviewed 
the runtime files.

http://cr.openjdk.java.net/~fzhinkin/8149374/webrev.01/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html

Why does this have three parameters?

- _index_map_p = new intArray(scratch_cp->length(), -1);
+ _index_map_p = new intArray(scratch_cp->length(), 
scratch_cp->length(), -1);

Why not just change it to:

     _index_map_p = new GrowableArray<int>(scratch_cp->length());

I don't see the three argument constructor to GrowableArray that takes 
-1 (??)

Is it possible to completely eliminate intArray, intStack, boolArray and 
boolStack, and the CHeapArray ?  If so array.hpp should really go in 
directory oops since the only Array<> left is for metaspace.   Maybe 
this can be a further cleanup?

Wow, thanks!

Coleen

On 3/31/16 11:14 AM, 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 
>>>>
>>>>
>>>