Hi Remi,
The keys used are based on the ordered names and types of "fields" in
the ObjectStreamClass that has been deserialized as part of the object
stream for the record "type" being deserialized. So for each record type
(Class<? extends Record>) there can be several distinct
ObjectStreamClasses deserialized in the same VM that were produced by
serializing different versions of record types in different VMs or
ClassLoader(s) and that may vary in the names and/or types and/or order
of fields. And since the MethodHandle chain of transformations that is
being used for a particular ObjectStreamClass is dependent on the
ordered names and types of ObjectStreamField(s) of deserialized
ObjectStreamClass, the key is based on that too.
You may ask why not simply add a MethodHandle field to the
ObjectStreamClass instance that is deserialized and therefore unique?
Well, such ObjectStreamClass only lasts for one "session" of
ObjectStream deserialization, so such caching wouldn't be efficient. But
common parts of that ObjectStreamClass that are only dependent on the
current local-VM type are cached in a special instance of
ObjectStreamClass and that's where I put a cache of deserializaiton
constructors in the form of a Map.
Regards, Peter
On 6/17/20 1:06 AM, Remi Forax wrote:
Hi Peter,
is their a reason to not use a ClassValue<MethodHandle> using the record class
as key instead of the more complex keys you are proposing ?
Rémi
----- Mail original -----
De: "Chris Hegarty" <chris.hega...@oracle.com>
À: "Peter Levart" <peter.lev...@gmail.com>
Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net>
Envoyé: Mardi 16 Juin 2020 17:15:03
Objet: Re: RFR: 8247532: Records deserialization is slow
Hi Peter,
On 14 Jun 2020, at 17:28, Peter Levart <peter.lev...@gmail.com> wrote:
...
[2]
http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.01/
I think that this is very good. It’s clever to build a chain of method handles
that can be invoked with the stream field values. This is a nice separation
between the shape of the data and the actual stream data.
The caching is on a per-stream-field shape basis, which should be consistent in
the common case, but of course is not always guaranteed to be the case, hence
the need for the cache. I think that this should be fine, since the
ObjectStreamClass ( that holds the cache ) is already itself cached as a weak
reference. But I did wonder if the size of this new cache should be limited.
Probably not worth the complexity unless it is an obvious issue.
All the serailizable records tests pass successfully with your patch. Good. I
did however notice that there is just a single test, DefaultValuesTest, that
exercises different stream shapes for the same class in the stream. Would be
good to expand coverage in this area, but of course some lower-level
test-specific building blocks will be needed help build the specially crafted
byte streams - I can help with this.
Overall I think that this change is good.
-Chris.
