RFR: JDK-8213371: GC/C2 abstraction and cleanup to handle custom offset for GC memory accesses
vladimir.kozlov at oracle.com
Tue Nov 6 21:24:56 UTC 2018
On 11/6/18 12:09 PM, Roman Kennke wrote:
> Hi Vladimir,
> Thanks for reviewing!
>> Changes in compile.cpp and type.cpp look fine.
>> I think in memnode.cpp (uint) casts are used to take into account
>> OffsetTop and OffsetBot without explicitly checking for them. We don't
>> expect OffsetTop here - there assert to check that. But 'off' could be
>> OffsetBot. In line 1706 there is OffsetBot check but not in line 1737:
> Ah! I was wondering. So by casting we implicitly accept OffsetBot as
> off_beyond_header=true. Ok.
>> I think you need to add explicit check OffsetBot for off_beyond_header.
>> You did that in type.cpp.
> Yes. However, I think it's clearer to include "|| off ==
> Type::OffsetBot" in the second use of off_beyond_header, rather than
> including it first, and then excluding it in the first use. Like this:
> I must admit that I don't really understand the usefulness of including
> OffsetBot in the 2nd clause, because that includes the case where offset
> is not actually beyond the mark word, iow 'we don't know what the offset
> is'. Right?
Bot offset is common case for array access - we have to take it into account.
Accesses to header should not be Bot (except LoadKlass as comment said - for which there are checks). Length and
markword should have fixed offsets.
>> On 11/5/18 7:34 AM, Roman Kennke wrote:
>>> There are a few places in C2 where we need to deal with Shenandoah's
>>> custom offset to load the forwarding pointer.
>>> This patch:
>>> - Adds hook and verification to allow GC to flatten/initialize an alias
>>> type for (e.g.) fwd ptr accesses (btw: the 'to' field in compile.cpp
>>> around the changed code is unused)
>>> - in memnode.cpp removes the (unnecessary?) cast from ints to uints
>>> - in type.cpp, make the offset check more explicit about offset > 0 or
>>> the two predefined offsets that could be negative.
>>> The latter two are related to the change insofar that they conflict with
>>> Shenandoah's -8 offset, which would require extra handling without those
>>> cleanups. Let me know if you want those changes separate.
>>> Testing: hotspot/jtreg:tier1
>>> Can I please get a review?
More information about the hotspot-gc-dev