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.
