Hi Peter,
199 private Map<ObjectStreamFieldsKey, MethodHandle> deserializationCtrs;
Change to be either ConcurrentMap or ConcurrentHashMap, thereby making it
clearer when using.
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.
> On Jun 16, 2020, at 8:15 AM, Chris Hegarty <[email protected]> wrote:
>
> Hi Peter,
>
>> On 14 Jun 2020, at 17:28, Peter Levart <[email protected]> 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.
>
