RFR [9]: default Serialization should issue a fence after reconstructing an Object with final fields
Chris Hegarty
chris.hegarty at oracle.com
Fri Feb 20 14:35:44 UTC 2015
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.hegarty at oracle.com
> <mailto:chris.hegarty at 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.hegarty at oracle.com <mailto:chris.hegarty at 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.hegarty at oracle.com <mailto:chris.hegarty at 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>
>
>
More information about the core-libs-dev
mailing list