RFR: 8224661: Parallel GC: Use WorkGang (3: UpdateDensePrefixAndCompactionTask)

[Thomas Schatzl]

Hi,

  http://cr.openjdk.java.net/~lkorinth/workgang/1/_8224661-Parallel-GC-Use-WorkGang-3-UpdateDensePrefixAndCompactionTask-fixup-2 looks good.

Thanks,
  Thomas

----- Original Message -----
From: leo.korinth at oracle.com
To: kim.barrett at oracle.com
Cc: hotspot-gc-dev at openjdk.java.net
Sent: Wednesday, July 3, 2019 4:47:21 AM GMT -08:00 US/Canada Pacific
Subject: Re: RFR: 8224661: Parallel GC: Use WorkGang (3: UpdateDensePrefixAndCompactionTask)



On 03/07/2019 01:55, Kim Barrett wrote:
>> On May 27, 2019, at 4:21 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>> […]
>> Enhancement:
>>   https://bugs.openjdk.java.net/browse/JDK-8224661
>>
>> Webrev:
>> http://cr.openjdk.java.net/~lkorinth/workgang/0/_8224661-Parallel-GC-Use-WorkGang-3-UpdateDensePrefixAndCompactionTask/
>>   http://cr.openjdk.java.net/~lkorinth/workgang/0/all/
>>
>> Testing (on the whole patch series):
>>   mach5 remote-build-and-test --build-profiles linux-x64,linux-x64-debug,macosx-x64,solaris-sparcv9,windows-x64 --test open/test/hotspot/jtreg/:hotspot_gc -a -XX:+UseParallelGC
>>   gc test suite
>>
>> Thanks,
>> Leo
> 
> http://cr.openjdk.java.net/~lkorinth/workgang/1/_8224661-Parallel-GC-Use-WorkGang-3-UpdateDensePrefixAndCompactionTask/
> http://cr.openjdk.java.net/~lkorinth/workgang/1/_8224661-Parallel-GC-Use-WorkGang-3-UpdateDensePrefixAndCompactionTask-fixup-1/
> 
> Looks good.
> 
> A few minor comments.
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psParallelCompact.hpp
> 33 #include "gc/shared/taskqueue.hpp"
> 
> I don't see any changes to this file that need this #include.
> Presumably it's needed by some .cpp files, but they should be doing
> the #include.

Old artifact from older version where I used the shared task queue, 
thanks for finding this, I will remove it as it is not needed.

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psParallelCompact.cpp
> 2467     guarantee(_backing_array != NULL, "alloc failure");
> 
> Not needed; NEW_C_HEAP_ARRAY uses EXIT_OOM for the allocation failure
> mode.

Good catch, copied from other code that did it, will fix.

I have (also) created https://bugs.openjdk.java.net/browse/JDK-8227168 
to remove the source from where I got it. And fix some related problems.

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psParallelCompact.cpp
> 2618 static void compaction_with_stealing_task(ParallelTaskTerminator* terminator, uint which) {
> 
> s/which/worker_id/

will fix.

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psParallelCompact.cpp
> 2684   TaskQueue task_queue(last_space_id*(active_gc_threads * PAR_OLD_DENSE_PREFIX_OVER_PARTITIONING + 1));
> 
> Some breadcrumbs to explain that calculation would be really helpful.
> Maybe comments, or splitting up the calculation with helpful variable
> names, or something.

will add some comment.

> 
> Anyway, I think it's correct, but harder to verify than it could be.
> 
> ------------------------------------------------------------------------------
> 

Thanks,
Leo