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

[Filipp Zhinkin]

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