RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization

[Peter Levart]

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?

Regards, Peter

On 10/20/2016 07:57 PM, Roger Riggs wrote:
> Hi,
>
> Thanks for the comments..
>
> Webrev's updated in place with comments so far.  (Including David's 
> and Amy's)
>
> http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
>
> http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/ 
>
>
> 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 
>>> setAccessible.
>>> Tests are included for ReflectionFactory and the affected IIOP classes.
>>>
>>> Please review and comment,
>>>
>>> jdk repo webrev:
>>> http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
>>> corba repo webrev :
>>> http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/ 
>>>
>>>
>> 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).
> fixed
>>
>> 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 
> Serializable
> 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 supertype.
> 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.
> ok
>>
>> 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 
> initialization.
> Any thread using the ObjectStreamClass will use the same method handle 
> regardless of the
> thread calling the method.
>
> Thanks, Roger
>
>