On Tue, Jun 11, 2024 at 1:08 PM David Lloyd <david.ll...@redhat.com> wrote:
> > > On Tue, Jun 11, 2024 at 12:57 PM Alan Bateman <alan.bate...@oracle.com> > wrote: > >> >> >> On 11/06/2024 18:19, David Lloyd wrote: >> >> : >> >> I thought that might be where Alan was headed with this. I would support >> this solution; it would solve the problem for conformant serialization >> libraries. If a class has a `readObject`/etc. then we use it - we wouldn't >> care if it was "natural" or generated. This also gives us the option to >> allow the user to use `opens` selectively to opt-in to special >> optimizations, without a major penalty if they do not. >> >> Is there already someone assigned for this task >> >> >> Not to my knowledge so you have cycles to prototype and have these >> methods return a MH that work like a "default" readObject/writeObject then >> it would help the discussion. >> > > I think I can come up with *at least* a prototype in the next week or two. > I have one concern though. The `readObject` method, when defined by users, > must by specification do one of two things before it may read values from > the stream: > > * Call `OIS.defaultReadObject()` > * Call `OIS.readFields()` > > In the latter case, we don't have a problem, but if the user class calls > `defaultReadObject` then we're back in the boat of having to reflectively > set the fields of the class. We could possibly solve this by creating a > *new* accessor which creates a parallel version of the method which > *always* sets fields reflectively, even if a `readObject` already exists on > the class. I'm not sure if there is a better solution. There is a similar > problem on the write side, however this involves reading fields rather than > writing them (obviously). > I have a *very* rough prototype up [1]. It adds two new accessor methods to `ReflectionFactory`: `defaultReadObjectForSerialization` and `defaultWriteObjectForSerialization`. It was easier than expected, due largely to the classfile API, which is really nice. These methods create an artificial `readObject`/`writeObject` method in a hidden+nestmate class connected to the original class, which uses the stream's `GetField`/`PutField` to access the stream data and normal field operations to access the class fields. For usage, these methods can be used when `read|writeObjectForSerialization` return `null`, *or* when `defaultRead|WriteObject` is used. There is additionally a constant bootstrap temporarily located on `ReflectionFactory` to acquire a `MethodHandle` that can act as a setter for final fields (there is no JVM-breaking magic here; it uses regular public APIs to achieve this). However this constant bootstrap probably needs to be put somewhere in `java.base` to actually be useful (my test program needed `--add-reads` and `-add-opens` to make it work because `ReflectionFactory` is located in `jdk.unsupported`). I was hesitant to put it into `ConstantBootstraps` but maybe that's really the best place for it. The outstanding problems are: - Need to determine if this is a valid/approved approach - Move the field-setter constant bootstrap method to `java.base` somewhere (`j.l.i.ConstantBootstraps`?) - Field setter constant bootstrap does not have adequate access checking; couldn't figure out a good way to do it - Field setter constant bootstrap also works for non-final fields, though this is not used by the prototype - Caching - yea or nay? For now it's left up to the caller (I think this is consistent with existing methods but not sure) - There are some `ClassDesc`, `MethodHandleDesc`, and `MethodTypeDesc` that should be in constant fields - Probably some normal code improvements & cleanups - Documentation - Real tests - Probably a good idea to review the spec again to make sure no edge case was missed [1] https://github.com/dmlloyd/jdk/commit/serialization -- - DML • he/him
