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

Wed Apr 6 16:02:38 UTC 2016

Thank you, Coleen.

On Tue, Apr 5, 2016 at 3:40 PM, Coleen Phillimore
<coleen.phillimore at oracle.com> wrote:
>
> 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
>>>>>>>
>