Hi Mandy,
On 8/16/19 8:59 PM, Mandy Chung wrote:
Hi Roger,
Thanks for doing this. Replacing ancient bytecode generators makes
it much easier to support new features like value types.
The diff of ProxyGenerator is not quite right (might be due to
the rename?) as some code are unchanged but shown as modified.
Since so much of the file has changed, I took the opportunity to reorder
some of the methods and fields to more closely match current conventions.
"jdk.proxy.ProxyGenerator.saveGeneratedFiles" is an existing property.
Perhaps a better name for "jdk.proxy.proxy_15" would be
"jdk.proxy.ProxyGenerator.useASM" with default true.
Perhaps renaming ProxyGenerator_15 to LegacyProxyGenerator?
Since the property and old implementation should be removed
(and as soon as the risk is evaluated) it made more sense to name
the implementation with the version number of the class files (JDK
version 1.5) it produces.
The combo test is great that exercises different combinations.
It validates the interfaces that the proxy class implements and
the exception types in the methods of the proxy class.
I checked the existing test/jdk/java/lang/reflect/Proxy tests.
It looks like there is no proxy test with an invocation handler
that throws checked exceptions and also undeclared exceptions.
It'd be good if you can add that as a sanity test.
This does seem to be a legacy gap in the testing.
I'll write a test.
Thanks, Roger
Mandy
On 8/16/19 12:15 PM, Roger Riggs wrote:
Please review an enhancement to replace the java.lang.reflect.Proxy
class file generation.
The new generator uses ASM and generates stackmaps. The
implementation follows
the same structure as before but has many differences as it leverages
ASM for generating the bytecode.
A Combo test is included and two JMH based benchmarks.
The ancient ProxyGenerator_15 implementation is temporarily retained
to allow comparisons of generated class files and performance.
Issue:
https://bugs.openjdk.java.net/browse/JDK-8207814
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-upgrade-proxy-gen-8207814/
(Upgrading bytecode generation is necessary for Valhalla but makes
sense for the main line.)
Thanks, Roger
