RFR: 8247532: Records deserialization is slow

[Peter Levart]

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.hegarty at oracle.com>
>> À: "Peter Levart" <peter.levart at gmail.com>
>> Cc: "core-libs-dev" <core-libs-dev at 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.levart at 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.