[External] : Re: New candidate JEP: 471: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal

[David Lloyd]

On Wed, Jun 12, 2024 at 9:23 AM David Lloyd <david.lloyd at redhat.com> wrote:

>
>
> On Tue, Jun 11, 2024 at 1:08 PM David Lloyd <david.lloyd at redhat.com>
> wrote:
>
>>
>>
>> On Tue, Jun 11, 2024 at 12:57 PM Alan Bateman <Alan.Bateman at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240613/6bdbd465/attachment.htm>