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.hega...@oracle.com> wrote:
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.
