RFR (M): 8146987: Improve Parallel GC Full GC by caching results of live_words_in_range() [Was: Re: [PATCH] enhancement to ParallelScavenge Full GC]

[ray alex]

I guess I have no permission to edit code in webreview?
I wonder if I need to upload a new patch with the renaming? (based on
webreview code?)
Or may you(Thomas) rename these two variables?

Thanks!

2016-01-14 17:34 GMT+08:00 Mikael Gerdin <mikael.gerdin at oracle.com>:

> Hi,
>
> cc:ing Jon
>
> Jon, can you have a look at the logic in the live_words_in_range
> optimization performed here? I don't feel that I have enough knowledge
> about parallel compact to say if it's correct or not.
>
> On 2016-01-13 16:51, Thomas Schatzl wrote:
>
>> Hi Tianyang,
>>
>> On Sun, 2016-01-10 at 22:16 +0800, ray alex wrote:
>>
>>> Hi,
>>> By following the conventions, I remove the prefix "get_" and rename
>>> getter/setter to "last_query_*" in this patch version 4 (attached).
>>> Besides, I ran the measurements mentioned before (both for version 3
>>> and recheck this version 4 ), and there was no regressions.
>>>
>>> May I have your reviews on this patch?
>>>
>>> Thanks!
>>>
>>
>>   first, sorry for the delay, holidays and such...
>>
>> I created a CR for this enhancement at
>> https://bugs.openjdk.java.net/browse/JDK-8146987
>>
>> I also made some webrevs for this change that fix some minor issues:
>>
>> base patch that includes some compilation fixes:
>> http://cr.openjdk.java.net/~tschatzl/8146987/webrev.0/
>>
>> - recent changes introduced some merge errors in psParallelCompact,
>> mostly due to Unified logging
>> - the assert in psParallelCompact.cpp:3137 missed a parameter so the
>> sources did not compile in any debug mode.
>>
>> Some minor style fixes:
>> http://cr.openjdk.java.net/~tschatzl/8146987/webrev.1/ (full)
>> http://cr.openjdk.java.net/~tschatzl/8146987/webrev.0_to_1/ (diff)
>>
>
>
> 2151
> 2152   PSParallelCompact::AdjustPointerClosure closure(cm);
> 2153   PSParallelCompact::AdjustKlassClosure kclosure(cm);
> 2154
>
> Please name these oop_closure and klass_closure.
>
> Otherwise I think this looks good (modulo getting feedback from Jon)
>
> /Mikael
>
>
>
>
>
>
>> - made a few of the new methods private
>> - some coding style fixes: indentation, trailing white spaces, copyright
>> dates
>> - removed the new parMarkBitMap.inline.hpp again because its methods were
>> only called from parMarkBitmap.cpp, fixed includes to parMarkBitmap.cpp.
>> - prepare some push message, please check
>>
>> I did run internal gc testing on the .0 webrev (vm.gc testlist), and did
>> some performance verification (mainly on specjbb201x as this was what was
>> on hand). It does (as expected) improve the perf of full gc, although the
>> improvements are not as high as mentioned in your initial email, around 15%
>> average full gc time (for some rather specific configuration). That is
>> still quite good imo.
>>
>> Reproducing the results has been one reason why this reply took a while,
>> it needs a bit of carefully setting up tests to get results that are not
>> drowned in noise due to gc occurring on widely different heap contents, and
>> particularly ergonomics.
>> Not sure how you managed to do this with something like dacapo.
>> SPECjvm2008 in "realistic" configurations does not do a lot of (if any)
>> full gcs.
>>
>> The change looks good to me now, but I need to do some more testing again
>> with the .1 version, to make sure the cleanup did not introduce any
>> regressions anywhere.
>>
>> Thanks,
>>    Thomas
>>
>>
>


-- 
Lei Tianyang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160114/661aecdd/attachment.htm>