RFR: 8247532: Records deserialization is slow

[Peter Levart]

Hi Paul,

On 6/16/20 5:50 PM, Paul Sandoz wrote:
> Hi Peter,
>
>   199     private Map<ObjectStreamFieldsKey, MethodHandle> deserializationCtrs;
>
> Change to be either ConcurrentMap or ConcurrentHashMap, thereby making it clearer when using.


Sure. Will do that.


>
>
> 2679         private static MethodHandle defaultValueExtractorFor(Class<?> pType) {
>
> Can the implementation use MethodHandles,zero? e.g.
>
>    return MethodHandles.dropArguments(MethodHandles.zero(pType), 0, byte[].class, Object[].class);
>   
> Paul.


Yes, or even better: MethodHandles.empty(MethodType.methodType(pClass, 
byte[].class, Object[].class)) which was suggested by Johanes Kuhn as here:


http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.02/


Regards, Peter


>
>> On Jun 16, 2020, at 8:15 AM, Chris Hegarty <chris.hegarty at oracle.com> wrote:
>>
>> 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.
>>