On 24 Feb 2015, at 11:45, Peter Levart <[email protected]> wrote:
>> ... >>> - You are tracking the requiresFreeze flag in readSerialData() method for >>> each class slot the deserialized object is composed of. This can be >>> optimized and the 'hasFinalField' flag pre-computed for the whole object >>> (all slots) and stored in ObjestStreamClass as such. >> I don’t see how your proposed changes are any more performant ( all that has >> happened is that the call to hasFinal has been moved inside the loop in >> getClassDataLayout0 ), and it is more difficult, at least for me, to grok ( >> and has an additional context dependency ). Is your concern the reading of >> the requiresFreeze field in readSerialData? Would making this a local field >> in the loop address your concern? > > The loop in getClassDataLayout0 is only executed once per ObjectStreamClass > (the layout is then cached). The loop in readSerialData() is executed for > each object instance deserialized. Ok, got it now. > Considering the results of Alexey's research: > > http://shipilev.net/blog/2014/all-fields-are-final/ > > He came to conclusion that even in constructors that just set fields and do > no complex logic like deserialization, the relative overhead of freeze is > minimal. So we might be better off just issuing the freeze unconditionally > and not bother with tracking which might have more overhead even for very > small streams (for example one object with few ints). Yes, I read this before. Very interesting. >>> - We have to be careful with "loosening" of volatile writes to final fields >>> in custom readObject() methods (BigDecimal.intCompact for example) >>> especialy if they are writes to fields that are not serial fields in >>> ObjectStreamClass (either they are transient or not listed in >>> serialPersistentFields). By doing that, you are relying on the fact that >>> default deserialization (defaultReadObject() call in case of BigDecimal) >>> tracks at least one other final field that is also serial field. This is >>> the case with BigDecimal and BigInteger, but in general it is not. >> I agree, we need to be careful here, but as you say these two specific cases >> should be fine. > > And if the freeze is unconditional, we don't have to worry at all. If we can accept the cost of an unconditional freeze ( yet to be compared against the cost of tracking ), then this is a very nice property, arguably worth accepting a small cost for ( if only to prevent trying to reason about transient final fields ). >>> It will be interesting how tracking will be implemented most efficiently >>> when FieldAccess API appears, but that's another story… >> I think the work-in-progress FieldAccess API will really help here, but for >> now I’d like to proceed with changing these two cases, and they can be >> retrofitted when the new API is available. > > Agreed. > >> >>> So I propose the following variant for now (including just >>> ObjectInputStream and ObjectStreamClass) that fixes 1st two issues above. I >>> suggest waiting with BigDecimal/BigInteger changes until FieldAccess API is >>> available and throw away Unsafe usage alltogether at that point: >>> >>> http://cr.openjdk.java.net/~plevart/jdk9-dev/ObjectInputStream.freeze/webrev.01/ >> It was really helpful to have a webrev to put your comments in context. >> Thanks. >> >> Updated webrev, including all comments/changes so far: >> http://cr.openjdk.java.net/~chegar/deserialFence/webrev.03/webrev/ > > That's better now. Let me just try to measure the overhead of tracking on > simple objects to see if it actually pays off or is contra-productive in any > case. Thanks. I’ll see if I can get some measurements also. -Chris. > Regards, Peter > >> >> -Chris. >> >> >>> Regards, Peter >>> >>> >>>> 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.
