Ok Chris, sounds good. It could be, but I omitted it as it requires a pesky explicit assignment of > false in the case where there are not final fields!
You could use a local (non-final) boolean to track this state (assigned with false), and then set the field with it after the looping is over. Anyway, your call. On Mon, Feb 23, 2015 at 10:32 AM, Chris Hegarty <chris.hega...@oracle.com> wrote: > On 23/02/15 14:08, Vitaly Davidovich wrote: > >> Likewise, seems fine. >> > > Thanks Vitaly. > > 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? >> > > Probably not. The freeze should probably happen before the > ObjectInputValidation callback, as this justs opens another opportunity for > early publication of the object, but probably after the handle update and > check. > > Also, hasFinal field can be final, unless I missed some context in the >> webrev. >> > > It could be, but I omitted it as it requires a pesky explicit assignment > of false in the case where there are not final fields! > > For completeness, updated webrev in-place, which I intend to push, unless > there are further comments: > http://cr.openjdk.java.net/~chegar/deserialFence/webrev.02/webrev/ > > -Chris. > > sent from my phone >> >> On Feb 23, 2015 7:30 AM, "David Holmes" <david.hol...@oracle.com >> <mailto: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/ >> >> <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. >> >>
