<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><span style="font-family:Arial,Helvetica,sans-serif">On Wed, Jun 12, 2024 at 9:23 AM David Lloyd <<a href="mailto:david.lloyd@redhat.com">david.lloyd@redhat.com</a>> wrote:</span><br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div style="font-family:arial,helvetica,sans-serif"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 11, 2024 at 1:08 PM David Lloyd <<a href="mailto:david.lloyd@redhat.com" target="_blank">david.lloyd@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div style="font-family:arial,helvetica,sans-serif"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 11, 2024 at 12:57 PM Alan Bateman <<a href="mailto:Alan.Bateman@oracle.com" target="_blank">Alan.Bateman@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><u></u>
<div>
<br>
<br>
<div>On 11/06/2024 18:19, David Lloyd wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div style="font-family:arial,helvetica,sans-serif">:<br>
</div>
</div>
<div class="gmail_quote">
<div><br>
</div>
<div>
<div style="font-family:arial,helvetica,sans-serif">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.</div>
<div style="font-family:arial,helvetica,sans-serif"><br>
</div>
<div style="font-family:arial,helvetica,sans-serif">Is there
already someone assigned for this task</div>
</div>
</div>
</div>
</blockquote>
<br>
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.<br></div></blockquote><div><br></div><div style="font-family:arial,helvetica,sans-serif">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:</div><div style="font-family:arial,helvetica,sans-serif"><br></div><div style="font-family:arial,helvetica,sans-serif">* Call `OIS.defaultReadObject()`</div><div style="font-family:arial,helvetica,sans-serif">* Call `OIS.readFields()`</div><div style="font-family:arial,helvetica,sans-serif"><br></div><div style="font-family:arial,helvetica,sans-serif">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).</div></div></div></blockquote><div><br></div><div><div style="font-family:arial,helvetica,sans-serif">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.</div><div style="font-family:arial,helvetica,sans-serif"><br></div><div style="font-family:arial,helvetica,sans-serif">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.</div><div style="font-family:arial,helvetica,sans-serif"><br></div><div style="font-family:arial,helvetica,sans-serif">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.</div><div style="font-family:arial,helvetica,sans-serif"><br></div><div style="font-family:arial,helvetica,sans-serif">The outstanding problems are:</div><div style="font-family:arial,helvetica,sans-serif">- Need to determine if this is a valid/approved approach</div><div style="font-family:arial,helvetica,sans-serif">- Move the field-setter constant bootstrap method to `java.base` somewhere (`j.l.i.ConstantBootstraps`?)</div><div style="font-family:arial,helvetica,sans-serif">- Field setter constant bootstrap does not have adequate access checking; couldn't figure out a good way to do it</div><div style="font-family:arial,helvetica,sans-serif">- Field setter constant bootstrap also works for non-final fields, though this is not used by the prototype</div><div style="font-family:arial,helvetica,sans-serif">- Caching - yea or nay? For now it's left up to the caller (I think this is consistent with existing methods but not sure)</div><div style="font-family:arial,helvetica,sans-serif">- There are some `ClassDesc`, `MethodHandleDesc`, and `MethodTypeDesc` that should be in constant fields</div><div style="font-family:arial,helvetica,sans-serif">- Probably some normal code improvements & cleanups</div><div style="font-family:arial,helvetica,sans-serif">- Documentation</div><div style="font-family:arial,helvetica,sans-serif">- Real tests</div><div style="font-family:arial,helvetica,sans-serif">- Probably a good idea to review the spec again to make sure no edge case was missed<br></div></div></div></div></blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">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.</div></div><div><br></div></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr">- DML • he/him<br></div></div></div>