> On Dec 4, 2015, at 5:47 AM, Paul Sandoz <paul.san...@oracle.com> wrote:
> 
>> 
>> On 3 Dec 2015, at 22:33, Mandy Chung <mandy.ch...@oracle.com> wrote:
>> 
>> 
>>> On Nov 26, 2015, at 8:22 AM, Paul Sandoz <paul.san...@oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> I have updated the patches:
>>> 
>>> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-jdk/webrev/
>>> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-hotspot/webrev/
>>> 
>>> There is now more documentation on Reference (copied and suitable 
>>> rearranged from 166 Fences.java). The method name remains the same.
>>> 
>> 
>> I think the addition to the Reference class specification should belong to 
>> the reachabilityFence method specification.  Any reason why not?
> 
> I thought it would be more visible in the JavaDoc, as it’s there upfront. The 
> api note may get larger if we include some additional real world examples. I 
> don’t have a strong opinion on this, if yours is stronger i will move it :-)
> 

Reference is the best class among other choices to place this reachabilityFence 
method and not directly tied with the Reference class spec. That’s how I read 
it.  I I think no issue for a method spec contains a long api note.  I prefer 
it to move to the method spec.

> 
> 
>> Should the reachabilityFence method throw NPE if ref is null?
>> 
> 
> I am ok with it doing nothing, it’s a performance sensitive method. It means 
> no null checks/de-opts are required and (hand-waving here...) might make it 
> more amenable to optimization see 
> https://bugs.openjdk.java.net/browse/JDK-8130398).
> 

I later also thought performance sensitivity that explain the reason to accept 
null.  It’s fine with me.

> 
> Updated:
> 
>  
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-jdk/webrev/
> 
> reachabilityFence is now annotated with @DontInline (to be pushed real soon 
> now) and the HotSpot changes are no longer needed.

Looks fine with me.  No need to see any new webrev.
Mandy

Reply via email to