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

[Erik Helin]

On 11/16/2016 01:00 AM, 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 oracle.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.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8166811
>
> Webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8166811/webrev.01/

First of all, thanks for doing this tricky work. One initial comment:

659   // Iterate over the objects overlapping the card designated by
660   // card_ptr, applying cl to all references in the region.  This
661   // is a helper for G1RemSet::refine_card, and is tightly coupled
662   // with it.

In the first sentence you mention the now removed argument card_ptr. 
Maybe just reword this to "Iterate over the objects covered by the 
memory region, applying cl to all references in the region"?

I will have to sleep on this review, the synchronization to make all of 
this hold together seems to be all over place :) (not your fault, 
pre-existing). I will continue this review tomorrow morning with a fresh 
brain.

Thanks,
Erik

> 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.
>