Likewise, seems fine. By the way, is there a reason not to call freeze() right before returning obj? Is there something special about the place it's invoked at now?
Also, hasFinal field can be final, unless I missed some context in the webrev. sent from my phone On Feb 23, 2015 7:30 AM, "David Holmes" <david.hol...@oracle.com> wrote: > Hi Chris, > > On 23/02/2015 9:01 PM, Chris Hegarty wrote: > >> Peter, David, Vitaly, >> >> Can you please take a look at the latest version of this change: >> >> http://cr.openjdk.java.net/~chegar/deserialFence/webrev.02/webrev/ >> > > It seems reasonable but I don't have a clear picture of the connection > between readObject and readSerialData. > > David > > On 20/02/15 15:09, Peter Levart wrote: >> >>> ... >>> This looks good now. But I wonder if issuing fences after nested calls >>> to readObject() makes much sense. If cycles are present in a subgraph >>> deserialized by nested call to readObject(), a field pointing back to an >>> object not part of the subgraph stream will point to the object that >>> will not be fully initialized yet, so nested calls to readObject() >>> should not be expected to return a reference to a fully constructed >>> subgraph anyway. Only top-level call is guaranteed to return a fully >>> constructed graph. >>> >> >> Right. I was never convinced of this myself either. Removed. Unnecessary >> complication. >> >> If you optimize this and only issue one fence for top-level call to >>> readObject(), tracking of 'requiresFence' (I would call it >>> requiresFreeze to be in line with JMM terminology - the fence is just a >>> >> >> 'requiresFreeze' is better. Updated >> >> way to achieve freeze) can also be micro-optimized. You currently do it >>> like this: >>> >>> 1900 requiresFence |= slotDesc.hasFinalField(); >>> >>> which is a shortcut for: >>> >>> requiresFence = requiresFence | slotDesc.hasFinalField(); >>> >>> ...which means that the field is overwritten multiple times >>> unnecessarily and slotDesc.hasFinalField() is called every time. You can >>> write the same logic as: >>> >>> if (!requiresFence && slotDesc.hasFinalField()) { >>> requiresFence = true; >>> } >>> >> >> ... and it is more readable. Updated. >> >> There will be at most one write to the field and potentially less calls >>> to slotDesc.hasFinalField(). >>> >> >> -Chris. >> >
