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.
