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

[Roger Riggs]

Hi Peter,

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.
Will do.
>
> 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.

Roger

>
> 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
>>
>>
>