[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 5043030 (reflect) unnecessary object creation in reflection
From:       Andrej Golovnin <andrej.golovnin () gmail ! com>
Date:       2014-06-12 19:03:53
Message-ID: CAM6ZMz8wGhNT+QbxPKt46Zpv34cPLe4GwMiheqk1GnKnj=f5Vw () mail ! gmail ! com
[Download RAW message or body]

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
> jdk_lang which includes reflection with
> 
> make TEST=jdk_lang {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


[prev in list] [next in list] [prev in thread] [next in thread]