JDK 9 RFR of JDK-8035279: Clean up internal deprecations in BigInteger

Paul Sandoz paul.sandoz at oracle.com
Tue Feb 25 12:26:21 UTC 2014


On Feb 25, 2014, at 1:32 AM, Brian Burkhalter <brian.burkhalter at oracle.com> wrote:

> 
> On Feb 20, 2014, at 6:39 PM, David Holmes wrote:
> 
>> Not clear what you mean by this.
> 
> This is more or less my reaction to this entire thread, so to speak. ;-) Anyway, thanks for all the comments.
> 

:-)


> Note that I am ignoring the powerCache field comments for the moment.
> 
> To distill the discussion down to just the proposed changes I posted, my reading is that there is more or less consensus on two points:
> 
> 1) the instance fields in question *should* be volatile for this proposed change set

Yes. Sorry, i mislead you earlier, i got confused about what hotspot does (and realistically most other platforms) compared to what is currently specified (and hopefully rectified in the JMM update).


> 2) non-zero initial values should be avoided in case of instance leaking to non-constructing threads
> 

If say, in your patch, the bitCount field was not volatile then there is a possibility on some platforms that a non-constructing thread may observe default values:

class X {
  static BigInteger x;

  static void threadOne() {
    x = new BigInteger("11111111", 2);
    // A compiler is allowed to move code such that the put to x can occur before the put to bitCount

    // If bitCount is volatile then a memory barrier or fence is placed around the put of bitCount 
    // such that (in general) code above cannot be moved below and code below cannot be moved above
    // (in addition to other things going with the caches on the processors to ensure visibility)
  }

  static void threadTwo() {
    if (x != null) {
      int bc = x.bitCount();
      assert bc = 8;  // may fail, could return 0   
    }
  }
}

I tweaked the example from http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html (although 10 years old that article is one of the best around at explaining things).

If you leak the "this" reference to another thread during construction then all bets are off w.r.t. the visibility of any final fields.

Note the use of putVolatile on the final fields when deserializing, this ensures fences are in place so that the visibility of the final fields is preserved [*].


> Is this accurate?
> 
> On second thought it occurred to me that instead of any complicated or contentious changes, as it were, the ugliness I was trying to remove from the code could just as well be addressed by simply changing the names of the affected instance variables to indicate what their respective values really represent, e.g., "bitCount" becomes "bitCountPlusOne" and we remove the @Deprecated and @deprecated. Yeah this is still ugly ...
> 

Might as well just remove the @Deprecated stuff. I think it is fine under the circumstances to have offsets and getter methods that return the correct values.

Paul.

[*] Strictly speaking it should only be necessary to do a put*Volatile on the last final field set, others can use put*. And, unfortunately this is one use-case where developers need to use Unsafe. Even if we have fence method on Objects the final field still needs to be set and reflection is currently slower, although MethodHandles could be an alternative when using a trusted Lookup instance.


More information about the core-libs-dev mailing list