Hi Chris,
On 02/24/2015 11:53 AM, Chris Hegarty wrote:
Peter,
On 23 Feb 2015, at 21:08, Peter Levart <[email protected]> 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.
