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

[David Holmes]

On 24/02/2015 7:08 AM, Peter Levart 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.
>
> - 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.
>
> - 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. It
> will be interesting how tracking will be implemented most efficiently
> when FieldAccess API appears, but that's another story...
>
> 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/

1183             hasFinal |= d.fieldRefl.hasFinal();

Didn't you suggest not using this form previously due to its inefficiency.

David


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