RFR [9]: default Serialization should issue a fence after reconstructing an Object with final fields

[Chris Hegarty]

On 24 Feb 2015, at 11:45, Peter Levart <peter.levart at gmail.com> 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.