RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization
Roger.Riggs at Oracle.com
Fri Oct 21 14:26:33 UTC 2016
Thanks for the review and suggestions.
Will update the webrev shortly.
On 10/21/2016 3:08 AM, Peter Levart wrote:
> Hi Roger,
> I don't know whether the performance of invoking
> sun.reflect.ReflactionFactory methods matters very much, but you could
> do a nit to help JIT optimize their invocation better. As both
> jdk.internal.reflect.ReflectionFactory and the
> sun.reflect.ReflectionFactory are singletons, you could use the
> strategy of sun.misc.Unsafe which references the delegate instance
> (theInternalUnsafe) off a static final field instead of instance final
> field. JIT can constant-fold such reference then.
> Otherwise I think it is a good strategy to give frameworks limited
> access to selected private methods targeted for a special purpose -
> serialization in this case, instead of opening the doors for generic
> reflection (via setAccessible). It just feels a little strange that
> this is new API in an "unsupported" module which destiny is to
> eventually "fade-away" once required functionality appears in a
> supported way. What is the message?
Alan answered already.
> Regards, Peter
> On 10/20/2016 07:57 PM, Roger Riggs wrote:
>> Thanks for the comments..
>> Webrev's updated in place with comments so far. (Including David's
>> and Amy's)
>> On 10/20/2016 8:21 AM, Alan Bateman wrote:
>>> On 19/10/2016 20:59, Roger Riggs wrote:
>>>> The support in sun.reflect.ReflectionFactory for custom
>>>> serialization, such as IIOP input
>>>> and output streams, is being expanded beyond the necessary
>>>> constructor of a serializable
>>>> class to include access to the private methods readObject,
>>>> writeObject, readResolve,
>>>> writeReplace, etc.
>>>> The IIOP implementation is updated to use a combination of
>>>> ReflectionFactory and
>>>> Unsafe to serialize and deserialize objects and no longer rely on
>>>> Tests are included for ReflectionFactory and the affected IIOP
>>>> Please review and comment,
>>>> jdk repo webrev:
>>>> corba repo webrev :
>>> I skimmed through the changes.
>>> I assume findReadWriteObjectForSerialization should throw
>>> InternalError, rather than return null, if IllegalAccessException is
>>> thrown (as IAE is not possible here).
>>> You've added @since 9 to sun.reflect.ReflectionFactory but that
>>> class is not new.
>> It was new in 9, but did not previously have @since. Its internal so
>> it may not be important.
>> Added by 8137058: Clear out all non-Critical APIs from sun.reflect
>>> The javadoc for
>>> sun.reflect.ReflectionFactory.newConstructorForSerialization doesn't
>>> say that it returns null when the Class is not Serializable.
>> The current implementation does not check that the argument is
>> and there are tests for that. I will update the documentation to not
>> specify a Serializable class.
>> I would need to track down all the uses to understand that adding the
>> check would not break something.
>> It also does not check that the requested constructor is for a
>> I think the semantics of newConstructorForSerialization should
>> include the search for
>> the super-type's noarg constructor instead of IIOP doing that search.
>>> For the MH returning methods then I assume the javadoc should say
>>> that it returns a direct method handle.
>>> The synchronization in the IIOP ObjectStreamClass isn't very clear.
>>> Are the invoke*, read*, write* methods all invoked by the same
>>> thread that creates the ObjectStreamClass with the lookup method?
>> ObjectStreamClass instances are cached and re-used across all threads.
>> ObjectStreamClass.init() handles the synchronization of the
>> Any thread using the ObjectStreamClass will use the same method
>> handle regardless of the
>> thread calling the method.
>> Thanks, Roger
More information about the core-libs-dev