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

Wed Apr 6 16:06:05 UTC 2016

Thank you, Vladimir.

Here is the latest webrev (I've update array.hpp &
parNewGeneration.cpp according to Mikael's comments):
http://cr.openjdk.java.net/~fzhinkin/8149374/webrev.03/

Regards,
Filipp.

On Tue, Apr 5, 2016 at 8:44 PM, Vladimir Ivanov
<vladimir.x.ivanov at oracle.com> wrote:
> Filipp, I'll sponsor the change.
>
> Thanks for taking care of it.
>
> Best regards,
> Vladimir Ivanov
>
>
> On 4/5/16 3:40 PM, Coleen Phillimore 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
>>>>>>>>
>>>>>>>>
>>
>