RFR: 8220465: Use shadow regions for faster ParallelGC full GCs
thomas.schatzl at oracle.com
Mon Nov 18 14:33:00 UTC 2019
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:
As indicated in the URL, these are merely suggestions.
- 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.
"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
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
- 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
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
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
"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.
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
- 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.
More information about the hotspot-gc-dev