RFR: 8220465: Use shadow regions for faster ParallelGC full GCs

[Thomas Schatzl]

Hi all,

On 16.11.19 06:13, Haoyu Li wrote:
> Hi Stefan,
> 
> Thanks very much for all your reviewing effort! Please feel free to contact
> me if there is any problem. Looking forward to hearing from you!
> 
> Best Regards,
> Haoyu Li
> 
> Stefan Johansson <stefan.johansson at oracle.com> 于2019年11月15日周五 下午5:21写道：
> 
>> Hi Haoyu,
>>
>> Thanks for the updates, here are new webrevs:
>> Full: http://cr.openjdk.java.net/~sjohanss/8220465/03/
>> Inc: http://cr.openjdk.java.net/~sjohanss/8220465/02-03/
>>
>> I have asked more people to look at the change now, so you might await
>> some more feedback.
>>

There were a few style issues that I am not all listing below. I also 
started renaming quite a few methods because they were (imo) misnamed, 
did not capture the intent of the method, and I have the strong 
suspicion that the same things are named differently at times - at least 
as far as I understood.

That makes reviewing very confusing and needs to be cleaned up.

My initial changes are available at the following link, but it became 
clear to me that this needs more care:

http://cr.openjdk.java.net/~tschatzl/8220465/webrev.03.suggestions/

As indicated in the URL, these are merely suggestions.

Other notes:

- static const ints can be initialized in the definition (UNUSED, 
SHADOW, ...); also they should be CamelCased; they are very unspecific 
too - I added some prefix to distinguish them a bit.

- the documentation about this change is imho lacking.

  - It would be nice to explain the idea of shadow regions somewhere 
assuming that you know how parallel works. Including the reference to 
the paper. :)

  - some of the comments just show what code (often a single statement) 
does, not the what and why or the reason why a particular method or 
member exists. Or explains one or the other.

E.g.

"The shadow region array, we use it in a LIFO fashion, so that we can 
reuse shadow regions for better data locality and utilization"

   - at this point we have no idea what a "shadow region" is and we 
can't find out easily because it is called "shadow record" or "steal 
record" elsewhere.

Something better could be:

"Contains currently free shadow regions (assuming we converge on that 
name). We use it in a LIFO fashion for better data locality and 
utilization."

- I think there is a missed optimization opportunity in (now) 
PSParallelCompact::initialize_shadow_regions(). There, the code 
initializes the "free" region ids to region_at_top+1 to end_region of a 
particular space.

If the top for a given space is at a region boundary (e.g. if a space is 
empty, which is probably common for one of the survivor spaces), you 
loose a single region per space.

One reason might that the code uses region "0" as sentinel to indicate 
"there is no shadow region available" in 
ParCompactionManager::acquire_shadow_region().

This could be fixed by improving the code in 
PSParallelCompact::initialize_shadow_regions() and use a sentinel region 
value of (size_t)~0 (as an explicit constant).

Even if you do not change this, please introduce an explicit constant 
for this sentinel value. This makes the code more self-explanatory.

- at least in ParallelCompactData::RegionData::try_steal I would add a 
dirty read of the _shadow_state to avoid the overhead of obviously 
unsuccessful steal attempts (I do not know about frequencies of those, 
so ymmv, but probably it would be easiest to add it everywhere).

Also all the cmpxchg can/should use memory_order_relaxed to avoid the 
two full fences every time accessed as far as I can tell.

- not sure about whether "acquire_shadow_region()/release_shadow_region" 
are good names for 
"PSParallelCompact::try_pop_shadow_region/push_shadow_region" (or 
something else).

"Acquire"/"release" has a very specific semantic related to a completely 
different area (memory ordering in MP systems), so we should probably 
avoid using them. There are other well-used pairs of names to add and 
remove elements to a container too.

- the changes in PSParallelCompact sometimes use the terms 
"steal_record", "shadow_record" and "shadow_region" (e.g. 
_shadow_region_array) interchangeably.

Can you give a reason for this? I am good with any (with a preference 
for "shadow_region" since it gives an idea of the contents while 
"record" is quite generic), but it makes reading the code harder than 
necessary.

- the names of the new methods e.g. in PSParallelCompact::RegionData 
should be more precise; e.g. please add what does "try_push" wants to 
push? Or "try_steal" steal?
Not even the comments for these contain that information, and I believe 
that by better naming of the methods, we can avoid the comments 
completely in most cases.

I *think* the change is good otherwise, but I'm constantly in need of 
referencing back to the definitions of the members/methods when looking 
through it, which makes me a bit uneasy. So I would like to ask you to 
improve above points first before I can give my go-ahead, and then I 
will look at it again.

Thanks,
   Thomas