Chris, I think it's fine if there are intra graph object references during deserialization - nothing unexpected should happen since intra thread semantics will be preserved. The freeze action is only a concern once deserialization completes and user code may publish an object from the graph unsafely to another thread; that's the only place where the fence is needed, I think.
sent from my phone On Feb 20, 2015 6:28 AM, "Chris Hegarty" <chris.hega...@oracle.com> wrote: > On 20/02/15 00:27, David Holmes wrote: > >> On 20/02/2015 10:13 AM, Vitaly Davidovich wrote: >> >>> In addition to Peter's comment, full fence seems unnecessarily strong and >>> will cause performance issues (especially if the fence is per object >>> in the >>> graph). A storeFence should be sufficient here, no? >>> >> >> It should be a fence per graph, or perhaps branches thereof, not per >> object. >> > > My original intent was to replicate the final-freeze action as performed > by constructors. What I am hearing is that we do better, without any > visible side-effect. Since the graph is not published until > readObject/Unshared returns, the fence can be added there. > > That said, it may be possible for one leaf in the graph to reference > another, but to observe this ( in readObject ), there would have an > implicit dependency on re-constructor order, which is fragile, at best. > > But yes a storeFence (horrible terminology :( ) would suffice given that >> the freeze action in constructors is only OrderAccess::storestore(). And >> Unsafe.storeFence() is OrderAccess::release() which is a >> storestore|storeload barrier. >> > > Thanks, updated. > > Updated Webrev: > http://cr.openjdk.java.net/~chegar/deserialFence/webrev.01/webrev/ > > Note, the changes in this webrev are overly defensive in the face of > recursive calls to readObject/Unshared. This should be ok, but probably not > strictly necessary. > > -Chris. > > > David >> >> >>> sent from my phone >>> On Feb 19, 2015 11:32 AM, "Chris Hegarty" <chris.hega...@oracle.com> >>> wrote: >>> >>> Additional note ( forgotten from original mail): >>>> >>>> The fence is needed for "final-freeze" is a one-off barrier at the >>>> end of >>>> deserialization, similar that of constructors . Under normal >>>> circumstances >>>> the object being deserialized is not visible until deserialization >>>> completes, so you don't need a "freeze" until deserialization completes. >>>> >>>> -Chris. >>>> >>>> On 19 Feb 2015, at 16:25, Chris Hegarty <chris.hega...@oracle.com> >>>> wrote: >>>> >>>> A number of years ago there was a proposal to use Unsafe.put*Volatile >>>>> >>>> methods to set final fields during default deserialisation [1][2], >>>> but it >>>> never made it due to concerns about the potential negative impact of the >>>> additional fences. Now we have a, private, fences API in the platform, I >>>> think it is time to revisit this. >>>> >>>>> >>>>> Webrev: >>>>> http://cr.openjdk.java.net/~chegar/deserialFence/webrev.00/webrev/ >>>>> >>>>> Note: >>>>> - Section 17.5.3 in the JLS [3], “Freezes of a final field occur both >>>>> at the end of the constructor in which the final field is set, and >>>>> immediately after each modification of a final field via reflection >>>>> or other special mechanism.” I believe this is a consequence of >>>>> the way in which setting of final fields is supported in the public >>>>> API, Field.setAccessible(), ( as defined by JSR 133 ) and should >>>>> not restrict an implementation from using a more performant >>>>> means, as is suggested here. The statement in the JLS should >>>>> be revisited. >>>>> >>>>> - Default Serialization already has a dependency on Unsafe, and >>>>> I don’t see this additional dependency as making that any worse. >>>>> >>>>> - Open question, should we include volatile fields as well as final? >>>>> >>>>> - The changes in the webrev will issue a fence even if custom >>>>> deserialization is performed. I think this is ok, as it will be more >>>>> consuming to try to determine if a custom readObject set a final >>>>> through reflection, or not. >>>>> >>>>> -Chris. >>>>> >>>>> [1] https://bugs.openjdk.java.net/browse/JDK-6647361 >>>>> [2] >>>>> >>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2010- >>>> November/005292.html >>>> >>>> >>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2010- >>>> December/005456.html >>>> >>>> [3] >>>>> >>>> http://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5.3 >>>> >>>> >>>>
