RFR: 8166811: Missing memory fences between memory allocation and refinement

Thomas Schatzl thomas.schatzl at oracle.com
Wed Nov 16 09:06:54 UTC 2016


Hi Kim,

On Tue, 2016-11-15 at 19:00 -0500, Kim Barrett wrote:
> > 
> > On Nov 15, 2016, at 5:26 AM, Thomas Schatzl <thomas.schatzl at oracle.
> > com> wrote:
> > 
> > Hi,
> > 
> > On Thu, 2016-11-10 at 13:20 -0500, Kim Barrett wrote:
> > > 
> > > > 
> > > > 
> > > > On Nov 8, 2016, at 7:52 AM, Thomas Schatzl <thomas.schatzl at orac
> > > > le.c
> > > > om> wrote:
> > > > On Tue, 2016-10-25 at 19:13 -0400, Kim Barrett wrote:
> > > > - assuming this works due to other synchronization,
> > > This is the critical point.  There *is* synchronization there.
> > Okay, thanks. I just wanted to make sure that we are aware of that
> > we
> > are using this other synchronization here.
> > 
> > Thanks. Again I was mostly worried about noting this reliance on
> > previous synchronization down somewhere, even if it is only the
> > mailing
> > list.
> > 
> > It may be useful to note this in the code too. This would save the
> > next
> > one working on this code looking through old mailing list threads.
> > 
> > Maybe I am a bit overly concerned about making sure that these
> > thoughts are provided in the proper place though. Or maybe everyone
> > thinks that everything is clear :)
> I've updated some comments to mention that external synchronization.

 581   // The region could be young.  Cards for young regions are set
to
 582   // g1_young_gen, so the post-barrier will filter them
out.  However,
 583   // that marking is performed concurrently.  A write to a young
 584   // object could occur before the card has been marked young,
slipping
 585   // past the filter.

I would prefer if the text would not change terminology for the same
thing mid-paragraph, from "setting" to "marking". The advantage of it
reading better seems to be smaller than the potential confusion.

Everything else looks very nice.

Thanks for considering my comments.

> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8166811
> 
> Webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8166811/webrev.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8166811/webrev.01.inc/
> 
> Also, since this set of changes is rather intertwined with the
> changes
> for 8166607, here is a combined webrev for both:
> http://cr.openjdk.java.net/~kbarrett/8166811/combined.01/
> 
> I think I'll do as Erik suggested and push the two together.

Just fyi, you can push two commits at once, or one commit having two
CR-number lines.
I think it is sufficient to commit these two changes in a single push
job, but I do not see a need for making it a single commit.

Either way is fine with me.

Thanks,
  Thomas



More information about the hotspot-dev mailing list