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

[Jon Masamitsu]

http://cr.openjdk.java.net/~tschatzl/8146987/webrev.1/src/share/vm/gc/parallel/psCompactionManager.hpp.frames.html

Please initialize these in the constructor

112 HeapWord* _last_query_beg;
113 oop _last_query_obj;
114 size_t _last_query_ret;

http://cr.openjdk.java.net/~tschatzl/8146987/webrev.1/src/share/vm/oops/oop.hpp.udiff.html

Any reason to use "pc" as the parameter name here and
not "cm" as is used elsewhere?

- void pc_update_contents();
+ void pc_update_contents(ParCompactionManager* pc);


Jon

On 01/14/2016 01:34 AM, Mikael Gerdin wrote:
> 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
>>
>