RFR: 5043030 (reflect) unnecessary object creation in reflection (original) (raw)

Andrej Golovnin andrej.golovnin at gmail.com
Thu Jun 12 19:03:53 UTC 2014

Hi Joel,

First, thanks for contributing this. Lets start with some process, have you

signed the OCA?

Yes, you can find me here: http://www.oracle.com/technetwork/community/oca-486395.html#g

> > I would say, It depends on how do you define "matters". I personally don't care > about the native part in this case, as I always set "sun.reflect.noInflation=true". > But I think the changes to Hotspot are just needed for the correctness of the fix.

I won’t review the HotSpot change, and

David has already reviewed it and he didn't find any problems so far.

I don’t think it is that desirable given that we inflate after 15 calls, and it adds code to the VM.

As I already wrote it is just needed for the consistent behavior. I understand that JDK is Oracle's product and it is up to you to accept or reject some changes. But as a user of your product I expect consistent behavior even if that means to add 47 lines of code (the most of them are just a simple switch statement) to HotSpot. If you reject the HotSpot change, then you should be prepared to reject from time to time bug reports about inconsistent behavior of the Java Reflection API. I'll file the first one. :-D

That is why we have tests :) You will have an easier time getting this accepted it you can show good code coverage of the fix in the current test suite for example. See if you can get jcov [1] up and running with this patch, I haven’t tried it on classes in sun.reflect but if it works and you can prove good coverage it will make my life easier, which directly translates to how easy it will be to get this patch committed.

Do you have any documentation for jcov? The Wiki page doesn't contain any useful information about the usage of jcov.

Lets start with your current tests and the reflection tests in jdk/test and try to get a coverage metric. With the new build system, supply “—with-jtreg=some path” to configure and you can run a test suite called jdklang which includes reflection with

make TEST=jdklang {CONCURRENCY=[num physical cores/4]} test

Here are the results:

Tests that passed 440 Tests that failed 2 Tests that had errors 1 Tests that were not run 5186 Total 5629

The failed tests:

jdk/lambda/LambdaTranslationTest1.java jdk/lambda/LambdaTranslationTest2.java

From my standpoint, this tests have bugs. They make use of Locale sensitive APIs and assume that the current Locale has the same formatting rules as Locale.US. I use a locale with a different formatting rules. This tests fail even without my patch.

And here is the test which had errors (this test fails also without my patch):

sun/misc/CopyMemory.java

The test fails with the following error message:

Agent[1].stdout: # Agent[1].stdout: # A fatal error has been detected by the Java Runtime Environment: Agent[1].stdout: # Agent[1].stdout: # SIGSEGV (0xb) at pc=0x00000001029157cb, pid=76794, tid=16771 Agent[1].stdout: # Agent[1].stdout: # JRE version: OpenJDK Runtime Environment (9.0) (build 1.9.0-internal-andrej_2014_06_11_20_43-b00) Agent[1].stdout: # Java VM: OpenJDK 64-Bit Server VM (1.9.0-internal-andrej_2014_06_11_20_43-b00 mixed mode bsd-amd64 compressed oops) Agent[1].stdout: # Problematic frame: Agent[1].stdout: # V [libjvm.dylib+0x5157cb] Unsafe_SetByte+0x5b Agent[1].stdout: # Agent[1].stdout: # Failed to write core dump. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again Agent[1].stdout: # Agent[1].stdout: # An error report file with more information is saved as: Agent[1].stdout: # /Users/Andrej/Projects/jdk9/build/macosx-x86_64-normal-server-release/testoutput/jdk_lang/JTwork/scratch/0/hs_err_pid76794.log Agent[1].stdout: # Agent[1].stdout: # If you would like to submit a bug report, please visit: Agent[1].stdout: # http://bugreport.sun.com/bugreport/crash.jsp Agent[1].stdout: #

It seems to be this bug https://bugs.openjdk.java.net/browse/JDK-8022407. But the bug is closed as fixed. Maybe we have a small regression. Btw, I'm using a MacBook Pro with the latest versions of Mac OS X and Xcode and JDK 9 master repository. If needed I can provide more information.

While the changes to the field accessors look easy, I need to work my way through the entire FieldAccessor implementation. I’ll have to get back to you on that.

The summary in both tests could be improved, while the language mandates the caches, that doesn’t apply to reflection. Some comments: src/share/classes/sun/reflect/AccessorGenerator.java src/share/classes/sun/reflect/MethodAccessorGenerator.java looks fine from a casual glance. test/java/lang/reflect/Method/invoke/TestMethodReflectValueOf.java: this need to be redesigned when dropping the vm part. I think it could also be interesting to see that the return from a direct method call .equals() the return from a reflective call. You might also consider setting the inflation threshold just high enough that you can make one pass uninflated checking just .eqials() then one pass in inflated code checking == as well. Before I start to change something I think we should make the decision whether we are going to drop the VM part or not. As I already said, I'm for the consistent behavior and therefore against the dropping the VM part. But I'm only the user of the JDK. Maybe other members of the core team could share with us their opinion.

Best regards, Andrej Golovnin

More information about the core-libs-dev mailing list