Hi Chris,
On 6/16/20 5:15 PM, Chris Hegarty 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.
If there is a way for a test to compile several versions of the same
(record) class and then produce byte streams with it, then we could
simulate various class-evolution cases (field missing, surplus field,
change of field type, etc...) without crafting the bytestream by hand. WDYT?
Regards, Peter
