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

[Mikael Gerdin]

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
>