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

[Peter Levart]

Hi Chris,

On 02/24/2015 11:53 AM, Chris Hegarty wrote:
> Peter,
>
> On 23 Feb 2015, at 21:08, Peter Levart <peter.levart at gmail.com> wrote:
>
>> Hi Chris,
>>
>> On 02/23/2015 12: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/
>> There are still a couple of issues with this version:
>>
>> - You are issuing freeze action as soon as any readObject() invocation is complete (including nested invocations) when the invocation itself has set the requiresFreeze flag, which is cleared when freeze() is called, but can be set again for any other nested (sibling, ...) call to readObject(). So many freeze(s) can potentialy be issued. This can be fixed by checking for (level == 0) condition before calling freeze.
> Agreed. That is better.
>
> I noticed that you put the freeze after vlist.doCallbacks(). I originally wanted to freeze before the callbacks ( less chance of unsafe publication ), but now I think I agree with where you put it. The callbacks are to validate the graph, throwing an Exception if there is a problem, so we can avoid the freeze in this case.

Well, either way is ok considering current constraints. Exceptional path 
should not be concerned with overhead anyway.

>
>> - 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.

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).

>
>> - 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.

>
>> 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.

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.