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

[Peter Levart]

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/


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.