RFR: 8225716: G1 GC: Undefined behaviour in G1BlockOffsetTablePart::block_at_or_preceding

[Thomas Schatzl]

Hi,

On Mon, 2019-06-17 at 10:42 +0100, Andrew Haley wrote:
> On 6/14/19 8:54 PM, Peter B. Kessler wrote:
> > > Date: Fri, 14 Jun 2019 14:06:22 +0200
> > > From: Thomas Schatzl <thomas.schatzl at oracle.com>
> > > To: Andrew Haley <aph at redhat.com>, "hotspot-gc-dev
> > > openjdk.java.net"
> > > 	<hotspot-gc-dev at openjdk.java.net>
> > > Subject: Re: RFR: 8225716: G1 GC: Undefined behaviour in
> > > 	G1BlockOffsetTablePart::block_at_or_preceding
> > > 
> > > Hi,
> > > 
> > > On Thu, 2019-06-13 at 16:21 +0100, Andrew Haley wrote:
> > > > [...]
> > > > The bug has only ever been observed on 86-32, but it isn't a
> > > > target-specific bug. It's probably not specific to G1 either.
> > > 
> > > The other generational collector use similar code that apparently
> > > also
> > > assume volatile access in
> > > BlockOffsetArrayNonContigSpace::block_start_unsafe. While this is
> > > not
> > > issues for Serial obviously, it may be for Parallel/CMS and needs
> > > to be
> > > looked at too.
> > > [...]
> > 
> > ParallelGC only updates the block offset table when copying
> > objects.
> > Copying is to a destination claimed by a GC worker, so no other GC
> > thread is reading that part of the BOT while it is being written.
> > 
> > CMS does not move objects in the old generation, so the BOT for an
> > object does not change once the object is in place.  I don't know
> > what CMS does if it has to defragment the heap by running a
> > compacting collection.
> > 
> > If I recall correctly: it has been a while since I looked at any of
> > that code.  It would be a shame to make access to the BOT volatile/
> > atomic for the collectors that do not need that overhead.  Your
> > memory system will thank you for your attention to this detail.
> 
> Would it really matter? These are relaxed accesses, so there are no
> memory fence instructions emitted. The only difference would be that

This is unfortunately untrue for MSVC, which by default emits memory
fences on x64 [0]. Peter is probably referring to that.

However I believe that Hotspot code should not rely on that (and can
not since we provide much more platform support than Windows), and
actually enable /volatile:iso on Windows. That is, if possible, as this
may be a recent addition.

> rematerialization of a BOT element would be inhibited in the case
> where it could not be kept in a register. Instead, it'd have to be
> spilled, but only if registers were in very short supply. So, the
> cost is more or less nothing.

+1

Thanks,
  Thomas

[0] https://docs.microsoft.com/en-us/cpp/bui
ld/reference/volatile-volatile-keyword-interpretation?view=vs-2019