On 20/02/15 14:25, Vitaly Davidovich wrote:
Chris,
I think it's fine if there are intra graph object references during
deserialization - nothing unexpected should happen since intra thread
semantics will be preserved. The freeze action is only a concern once
deserialization completes and user code may publish an object from the
graph unsafely to another thread; that's the only place where the fence
is needed, I think.
Agreed.
I'm happy the way this has turned out. The changes are minimal are
resolve an issue that has been outstanding for some time now.
Thanks,
-Chris.
sent from my phone
On Feb 20, 2015 6:28 AM, "Chris Hegarty" <chris.hega...@oracle.com
<mailto:chris.hega...@oracle.com>> wrote:
On 20/02/15 00:27, David Holmes wrote:
On 20/02/2015 10:13 AM, Vitaly Davidovich wrote:
In addition to Peter's comment, full fence seems
unnecessarily strong and
will cause performance issues (especially if the fence is
per object
in the
graph). A storeFence should be sufficient here, no?
It should be a fence per graph, or perhaps branches thereof, not per
object.
My original intent was to replicate the final-freeze action as
performed by constructors. What I am hearing is that we do better,
without any visible side-effect. Since the graph is not published
until readObject/Unshared returns, the fence can be added there.
That said, it may be possible for one leaf in the graph to reference
another, but to observe this ( in readObject ), there would have an
implicit dependency on re-constructor order, which is fragile, at best.
But yes a storeFence (horrible terminology :( ) would suffice
given that
the freeze action in constructors is only
OrderAccess::storestore(). And
Unsafe.storeFence() is OrderAccess::release() which is a
storestore|storeload barrier.
Thanks, updated.
Updated Webrev:
http://cr.openjdk.java.net/~__chegar/deserialFence/webrev.__01/webrev/
<http://cr.openjdk.java.net/~chegar/deserialFence/webrev.01/webrev/>
Note, the changes in this webrev are overly defensive in the face of
recursive calls to readObject/Unshared. This should be ok, but
probably not strictly necessary.
-Chris.
David
sent from my phone
On Feb 19, 2015 11:32 AM, "Chris Hegarty"
<chris.hega...@oracle.com <mailto:chris.hega...@oracle.com>>
wrote:
Additional note ( forgotten from original mail):
The fence is needed for "final-freeze" is a one-off
barrier at the
end of
deserialization, similar that of constructors . Under normal
circumstances
the object being deserialized is not visible until
deserialization
completes, so you don't need a "freeze" until
deserialization completes.
-Chris.
On 19 Feb 2015, at 16:25, Chris Hegarty
<chris.hega...@oracle.com <mailto:chris.hega...@oracle.com>>
wrote:
A number of years ago there was a proposal to use
Unsafe.put*Volatile
methods to set final fields during default
deserialisation [1][2],
but it
never made it due to concerns about the potential
negative impact of the
additional fences. Now we have a, private, fences API in
the platform, I
think it is time to revisit this.
Webrev:
http://cr.openjdk.java.net/~__chegar/deserialFence/webrev.__00/webrev/
<http://cr.openjdk.java.net/~chegar/deserialFence/webrev.00/webrev/>
Note:
- Section 17.5.3 in the JLS [3], “Freezes of a
final field occur both
at the end of the constructor in which the
final field is set, and
immediately after each modification of a final
field via reflection
or other special mechanism.” I believe this is
a consequence of
the way in which setting of final fields is
supported in the public
API, Field.setAccessible(), ( as defined by JSR
133 ) and should
not restrict an implementation from using a
more performant
means, as is suggested here. The statement in
the JLS should
be revisited.
- Default Serialization already has a dependency on
Unsafe, and
I don’t see this additional dependency as making
that any worse.
- Open question, should we include volatile fields
as well as final?
- The changes in the webrev will issue a fence even
if custom
deserialization is performed. I think this is
ok, as it will be more
consuming to try to determine if a custom
readObject set a final
through reflection, or not.
-Chris.
[1]
https://bugs.openjdk.java.net/__browse/JDK-6647361
<https://bugs.openjdk.java.net/browse/JDK-6647361>
[2]
http://mail.openjdk.java.net/__pipermail/core-libs-dev/2010-__November/005292.html
<http://mail.openjdk.java.net/pipermail/core-libs-dev/2010-November/005292.html>
http://mail.openjdk.java.net/__pipermail/core-libs-dev/2010-__December/005456.html
<http://mail.openjdk.java.net/pipermail/core-libs-dev/2010-December/005456.html>
[3]
http://docs.oracle.com/javase/__specs/jls/se8/html/jls-17.__html#jls-17.5.3
<http://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5.3>
