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
