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

[Coleen Phillimore]

Filip, Thank you for your answers.  This change looks really good!!
Coleen

On 4/5/16 3:23 AM, Filipp Zhinkin wrote:
> Hi Coleen,
>
> thanks for taking a look at it.
>
> On Mon, Apr 4, 2016 at 11:21 PM, Coleen Phillimore
> <coleen.phillimore at oracle.com> wrote:
>> 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);
> GrowableArray won't initialize elements in backing array until you ask it to.
> And it also won't allow to access elements that were not initialized.
>
> So we have to pass three parameters there to allocate backing array
> and fill it with -1.
>
>> Why not just change it to:
>>
>>      _index_map_p = new GrowableArray<int>(scratch_cp->length());
> We use -1 there for CP entries that were not mapped during constant
> pools merging.
>
>> I don't see the three argument constructor to GrowableArray that takes -1
>> (??)
> It's the one that take init size, length, filler and few other
> implicit parameters:
> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/annotate/tip/src/share/vm/utilities/growableArray.hpp#l178
>
>> 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?
> I've already eliminated CHeapArray in the latest webrev [*],
> so only typedefs are preventing array.hpp movement.
>
> I'd prefer to eliminate typedefs and move array.hpp to oops directory
> in separate CR just
> to avoid webrev's growing and simplify reviewing.
> But if it's ok, then I can do it within this CR.
>
> Thanks,
> Filipp.
>
> [*] http://cr.openjdk.java.net/~fzhinkin/8149374/webrev.02/
>
>> 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
>>>>>>