RFR: 8166811: Missing memory fences between memory allocation and refinement
thomas.schatzl at oracle.com
Wed Nov 16 09:06:54 UTC 2016
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
582 // g1_young_gen, so the post-barrier will filter them
583 // that marking is performed concurrently. A write to a young
584 // object could occur before the card has been marked young,
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.
> 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
> for 8166607, here is a combined webrev for both:
> 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
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.
More information about the hotspot-dev