On Wed, Jun 12, 2024 at 9:23 AM David Lloyd <david.ll...@redhat.com> wrote:
> > > 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 > I've updated with a few commits today which solve (I think) the access control issue, moves constant descriptors to constants in a holder class, moves the bootstrap to `ConstantBootstraps`, just says "no" to caching, and cleans up a few other minor issues. I guess I'll go ahead and update JDK-8333796 to reference this new strategy and start preparing to put up a PR, unless someone raises an objection to the final approach taken here. -- - DML • he/him
