RFR: 5043030 (reflect) unnecessary object creation in reflection
Hi Joe, I have prepared a patch for the issue JDK-5043030. The patch consists of two parts: one for jdk and one for hotspot. You can find the webrevs here: JDK-patch: https://db.tt/7DYph0OH Hotspot-patch: https://db.tt/hHsN0yn4 The JDK-patch has two tests to verify the changes. Please review it and I hope you can sponsor it. Best regards, Andrej Golovnin
Hi, We need you send in the patches via the mailing list. Please reply to your mail with the diffs attached directly (not zipped for example). I’ll might have time to look at this next week, but it surprises me you need to patch hotspot. Allocations before we have inflated shouldn’t be a problem. Also do you have any data showing that this actually makes a difference? cheers /Joel On 28 maj 2014, at 23:44, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:
Hi Joe,
I have prepared a patch for the issue JDK-5043030. The patch consists of two parts: one for jdk and one for hotspot. You can find the webrevs here:
JDK-patch: https://db.tt/7DYph0OH Hotspot-patch: https://db.tt/hHsN0yn4
The JDK-patch has two tests to verify the changes.
Please review it and I hope you can sponsor it.
Best regards, Andrej Golovnin
Hi Joel,
We need you send in the patches via the mailing list. Please reply to your mail with the diffs attached directly (not zipped for example).
OK, will do that. But I always thought a webrev is the preferred and the best way to submit a patch.
I’ll might have time to look at this next week, but it surprises me you need to patch hotspot. Allocations before we have inflated shouldn’t be a problem. Also do you have any data showing that this actually makes a difference?
When you have for example following method: public int method() { return 0; } And you invoke this method over the reflection API, then the first N invocations are done via the native code. The returned value must be wrapped. Currently this is done by the following call java_lang_boxing_object::create(type, value, CHECK_NULL); in reflection.cpp. This method allocates a new object. As I wrote in my previous mail, the JDK patch contains two regression tests for jtreg to verify my changes. If you run this tests using current JDK9, than the tests will fail. After applying my patches the tests executed without any problem. Best regards, Andrej Golovnin
cheers /Joel
On 28 maj 2014, at 23:44, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:
Hi Joe,
I have prepared a patch for the issue JDK-5043030. The patch consists of two parts: one for jdk and one for hotspot. You can find the webrevs here:
JDK-patch: https://db.tt/7DYph0OH Hotspot-patch: https://db.tt/hHsN0yn4
The JDK-patch has two tests to verify the changes.
Please review it and I hope you can sponsor it.
Best regards, Andrej Golovnin
Hello, On 29 maj 2014, at 10:23, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:
Hi Joel,
We need you send in the patches via the mailing list. Please reply to your mail with the diffs attached directly (not zipped for example).
OK, will do that. But I always thought a webrev is the preferred and the best way to submit a patch.
You are right that webrevs are preferred for reviewing, but we also need the contribution submitted through openjdk infrastructure. BTW it looks like your attachments were stripped.
I’ll might have time to look at this next week, but it surprises me you need to patch hotspot. Allocations before we have inflated shouldn’t be a problem. Also do you have any data showing that this actually makes a difference?
When you have for example following method:
public int method() { return 0; }
And you invoke this method over the reflection API, then the first N invocations are done via the native code.
Yes, this is before inflation. Inflation happens after 15 calls IIRC, which is why I don’t think it matters from a performance standpoint.
The returned value must be wrapped. Currently this is done by the following call
java_lang_boxing_object::create(type, value, CHECK_NULL);
in reflection.cpp. This method allocates a new object.
As I wrote in my previous mail, the JDK patch contains two regression tests for jtreg to verify my changes. If you run this tests using current JDK9, than the tests will fail. After applying my patches the tests executed without any problem.
Your tests show that the valueOf caches are used which is good. However the byte code spinning is a core piece of reflection that is currently in production in countless places. A change in this area should not only be obviously correct and thouroghly tested, you must show "real world” benefit. Ideally you should be able to show significant reduction in allocation in some application. cheers /Joel
Hi Joel,
When you have for example following method:
public int method() { return 0; }
And you invoke this method over the reflection API, then the first N invocations are done via the native code.
Yes, this is before inflation. Inflation happens after 15 calls IIRC, which is why I don’t think it matters from a performance standpoint.
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.
Your tests show that the valueOf caches are used which is good. However the byte code spinning is a core piece of reflection that is currently in production in countless places. A change in this area should not only be obviously correct and thouroghly tested
That's why we do the code review. ;-)
, you must show "real world” benefit. Ideally you should be able to show significant reduction in allocation in some application.
I'm working on a product which has ca. 3 million lines of code and make direct and indirect use of Reflection API. And in one of our use cases I see, that JVM allocates ca. 9GB of Boolean objects. All of them are allocated through the usage of Reflection API. Even that most of this objects are allocated in TLABs and are removed by GC when the use case is finished, I would say we have allocated 9GB of Boolean objects to much. Or do you see it differently? Let me know, if you need more data or if I should write some test. If don't want accept the patch, then it's OK. But in this case you should close the issue 5043030 and explain why it won't be fixed. Best regards, Andrej Golovnin
Hi Andrej, The hotspot changes need to be reviewed by hotspot developers so I've cc'd the runtime team. On 29/05/2014 8:45 PM, Andrej Golovnin wrote:
Hi Joel,
When you have for example following method:
public int method() { return 0; }
And you invoke this method over the reflection API, then the first N invocations are done via the native code.
Yes, this is before inflation. Inflation happens after 15 calls IIRC, which is why I don’t think it matters from a performance standpoint.
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.
The valueOf calls may also allocate a new object so you can't just delete the JvmtiExport::post_vm_object_alloc call. Unfortunately you can't tell whether a new object was allocated or not. It is only for the smaller primitive types that any kind of Object caching is mandated. David -----
Your tests show that the valueOf caches are used which is good. However the byte code spinning is a core piece of reflection that is currently in production in countless places. A change in this area should not only be obviously correct and thouroghly tested
That's why we do the code review. ;-)
, you must show "real world” benefit. Ideally you should be able to show significant reduction in allocation in some application.
I'm working on a product which has ca. 3 million lines of code and make direct and indirect use of Reflection API. And in one of our use cases I see, that JVM allocates ca. 9GB of Boolean objects. All of them are allocated through the usage of Reflection API. Even that most of this objects are allocated in TLABs and are removed by GC when the use case is finished, I would say we have allocated 9GB of Boolean objects to much. Or do you see it differently?
Let me know, if you need more data or if I should write some test.
If don't want accept the patch, then it's OK. But in this case you should close the issue 5043030 and explain why it won't be fixed.
Best regards, Andrej Golovnin
Correction ... On 29/05/2014 9:06 PM, David Holmes wrote:
Hi Andrej,
The hotspot changes need to be reviewed by hotspot developers so I've cc'd the runtime team.
On 29/05/2014 8:45 PM, Andrej Golovnin wrote:
Hi Joel,
When you have for example following method:
public int method() { return 0; }
And you invoke this method over the reflection API, then the first N invocations are done via the native code.
Yes, this is before inflation. Inflation happens after 15 calls IIRC, which is why I don’t think it matters from a performance standpoint.
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.
The valueOf calls may also allocate a new object so you can't just delete the JvmtiExport::post_vm_object_alloc call. Unfortunately you can't tell whether a new object was allocated or not. It is only for the smaller primitive types that any kind of Object caching is mandated.
It is only for the smaller values (-128 to +127) of the integer primitives types (plus boolean) that caching is mandated. Float.valueOf and Double.valueOf always create objects. David
David -----
Your tests show that the valueOf caches are used which is good. However the byte code spinning is a core piece of reflection that is currently in production in countless places. A change in this area should not only be obviously correct and thouroghly tested
That's why we do the code review. ;-)
, you must show "real world” benefit. Ideally you should be able to show significant reduction in allocation in some application.
I'm working on a product which has ca. 3 million lines of code and make direct and indirect use of Reflection API. And in one of our use cases I see, that JVM allocates ca. 9GB of Boolean objects. All of them are allocated through the usage of Reflection API. Even that most of this objects are allocated in TLABs and are removed by GC when the use case is finished, I would say we have allocated 9GB of Boolean objects to much. Or do you see it differently?
Let me know, if you need more data or if I should write some test.
If don't want accept the patch, then it's OK. But in this case you should close the issue 5043030 and explain why it won't be fixed.
Best regards, Andrej Golovnin
Hi David,
The valueOf calls may also allocate a new object so you can't just delete the JvmtiExport::post_vm_object_alloc call. Unfortunately you can't tell whether a new object was allocated or not. It is only for the smaller primitive types that any kind of Object caching is mandated.
It is only for the smaller values (-128 to +127) of the integer primitives types (plus boolean) that caching is mandated. Float.valueOf and Double.valueOf always create objects.
You are right, that #valueOf call may allocate an object. But as far as I understand currently the JvmtiExport::post_vm_object_alloc call is only needed, because today the native code itself allocates an object by calling java_lang_boxing_object::create(type, value, CHECK_NULL);. My code changes this behavior and delegates object allocation back to Java by calling JavaCalls::call_static(&boxed_value, klass_handle, vmSymbols::valueOf_name(), valueOf_signature, &args, THREAD); But maybe I misunderstood the implementation of JavaCalls. Best regards, Andrej Golovnin
Hi Andrej, Sorry for the delay getting back to you. On 29/05/2014 10:24 PM, Andrej Golovnin wrote:
Hi David,
The valueOf calls may also allocate a new object so you can't just delete the JvmtiExport::post_vm_object_alloc call. Unfortunately you can't tell whether a new object was allocated or not. It is only for the smaller primitive types that any kind of Object caching is mandated.
It is only for the smaller values (-128 to +127) of the integer primitives types (plus boolean) that caching is mandated. Float.valueOf and Double.valueOf always create objects.
You are right, that #valueOf call may allocate an object. But as far as I understand currently the JvmtiExport::post_vm_object_alloc call is only needed, because today the native code itself allocates an object by calling java_lang_boxing_object::create(type, value, CHECK_NULL);.
Right, sorry - I was misunderstanding the purpose of the post_vm_object_alloc call: http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#VMObjectAlloc So from the perspective that you are diverting this back to Java code the hotspot changes look okay to me. The more general question, for the core-libs folk, is whether changing everything to use valueOf is overkill (given the limits of the required caching mechanisms) or good to do from a consistency perspective. I'm slightly on the overkill side of things but not enough to reject things. On the performance/benefit side, if I read things correctly you only see the 9GB of Boolean objects because you disable reflection-inflation - is that right? In that case, as Joel states, the gains are not really general, but on the other hand I don't see anything wrong with trying to improve the general efficiency here even if the greatest benefit comes from a "non-mainstream" usecase. David -----
My code changes this behavior and delegates object allocation back to Java by calling
JavaCalls::call_static(&boxed_value, klass_handle, vmSymbols::valueOf_name(), valueOf_signature, &args, THREAD);
But maybe I misunderstood the implementation of JavaCalls.
Best regards, Andrej Golovnin
Hi David,
Right, sorry - I was misunderstanding the purpose of the post_vm_object_alloc call:
http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti. html#VMObjectAlloc
So from the perspective that you are diverting this back to Java code the hotspot changes look okay to me.
I'm glad to hear that.
The more general question, for the core-libs folk, is whether changing everything to use valueOf is overkill (given the limits of the required caching mechanisms) or good to do from a consistency perspective. I'm slightly on the overkill side of things but not enough to reject things.
In case my vote counts, I'm for consistency. :-)
On the performance/benefit side, if I read things correctly you only see the 9GB of Boolean objects because you disable reflection-inflation - is that right?
No. The most of those objects are allocated by calls to java.lang.reflect.Field.get(). I disable reflection-inflation only on the server side just to save a little bit more memory. On the client side I'm still using reflection-inflation, because here is the startup time more important.
In that case, as Joel states, the gains are not really general, but on the other hand I don't see anything wrong with trying to improve the general efficiency here even if the greatest benefit comes from a "non-mainstream" usecase.
I would not call what we do a "non-mainstream" use case. We have a classic JEE application which make use of nearly all JEE APIs. All of those Boolean objects are allocated by a very famous JPA implementation, which use Reflection API to set/get properties of our domain objects. We have the same problem with Integer objects too. I brought the example with Boolean objects just because at least in the theory we should have per JVM only two instances of Boolean class. But in the reality we have to many of them. Best regards, Andrej Golovnin
Hi David, et.al. I would let the compiler do auto-boxing where necessary. (Assuming object identity is not necessary). If type disambiguation is necessary then use a cast to the target type and let the compiler do the rest. It keeps the source code simple and readable. But I don't think it is worth a proactive pervasive change. The gain is overshadowed by the overhead of the reviews. $.02, Roger On 6/1/2014 11:07 PM, David Holmes wrote:
Hi Andrej,
Sorry for the delay getting back to you.
On 29/05/2014 10:24 PM, Andrej Golovnin wrote:
Hi David,
The valueOf calls may also allocate a new object so you can't just delete the JvmtiExport::post_vm_object_alloc call. Unfortunately you can't tell whether a new object was allocated or not. It is only for the smaller primitive types that any kind of Object caching is mandated.
It is only for the smaller values (-128 to +127) of the integer primitives types (plus boolean) that caching is mandated. Float.valueOf and Double.valueOf always create objects.
You are right, that #valueOf call may allocate an object. But as far as I understand currently the JvmtiExport::post_vm_object_alloc call is only needed, because today the native code itself allocates an object by calling java_lang_boxing_object::create(type, value, CHECK_NULL);.
Right, sorry - I was misunderstanding the purpose of the post_vm_object_alloc call:
http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#VMObjectAlloc
So from the perspective that you are diverting this back to Java code the hotspot changes look okay to me.
The more general question, for the core-libs folk, is whether changing everything to use valueOf is overkill (given the limits of the required caching mechanisms) or good to do from a consistency perspective. I'm slightly on the overkill side of things but not enough to reject things.
On the performance/benefit side, if I read things correctly you only see the 9GB of Boolean objects because you disable reflection-inflation - is that right? In that case, as Joel states, the gains are not really general, but on the other hand I don't see anything wrong with trying to improve the general efficiency here even if the greatest benefit comes from a "non-mainstream" usecase.
David -----
My code changes this behavior and delegates object allocation back to Java by calling
JavaCalls::call_static(&boxed_value, klass_handle, vmSymbols::valueOf_name(), valueOf_signature, &args, THREAD);
But maybe I misunderstood the implementation of JavaCalls.
Best regards, Andrej Golovnin
Hi Roger, On 3/06/2014 12:15 AM, roger riggs wrote:
Hi David, et.al.
I would let the compiler do auto-boxing where necessary. (Assuming object identity is not necessary).
I don't see where the compiler comes into this one. ??? David -----
If type disambiguation is necessary then use a cast to the target type and let the compiler do the rest. It keeps the source code simple and readable.
But I don't think it is worth a proactive pervasive change. The gain is overshadowed by the overhead of the reviews.
$.02, Roger
On 6/1/2014 11:07 PM, David Holmes wrote:
Hi Andrej,
Sorry for the delay getting back to you.
On 29/05/2014 10:24 PM, Andrej Golovnin wrote:
Hi David,
The valueOf calls may also allocate a new object so you can't just delete the JvmtiExport::post_vm_object_alloc call. Unfortunately you can't tell whether a new object was allocated or not. It is only for the smaller primitive types that any kind of Object caching is mandated.
It is only for the smaller values (-128 to +127) of the integer primitives types (plus boolean) that caching is mandated. Float.valueOf and Double.valueOf always create objects.
You are right, that #valueOf call may allocate an object. But as far as I understand currently the JvmtiExport::post_vm_object_alloc call is only needed, because today the native code itself allocates an object by calling java_lang_boxing_object::create(type, value, CHECK_NULL);.
Right, sorry - I was misunderstanding the purpose of the post_vm_object_alloc call:
http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#VMObjectAlloc
So from the perspective that you are diverting this back to Java code the hotspot changes look okay to me.
The more general question, for the core-libs folk, is whether changing everything to use valueOf is overkill (given the limits of the required caching mechanisms) or good to do from a consistency perspective. I'm slightly on the overkill side of things but not enough to reject things.
On the performance/benefit side, if I read things correctly you only see the 9GB of Boolean objects because you disable reflection-inflation - is that right? In that case, as Joel states, the gains are not really general, but on the other hand I don't see anything wrong with trying to improve the general efficiency here even if the greatest benefit comes from a "non-mainstream" usecase.
David -----
My code changes this behavior and delegates object allocation back to Java by calling
JavaCalls::call_static(&boxed_value, klass_handle, vmSymbols::valueOf_name(), valueOf_signature, &args, THREAD);
But maybe I misunderstood the implementation of JavaCalls.
Best regards, Andrej Golovnin
Hi David, Sorry, crossed some email threads, in hotspot the compiler has no involvement. Changes to remove explicit wrapper types are being proposed in the libraries where the compiler can handle the boxing. Roger On 6/2/2014 10:19 PM, David Holmes wrote:
Hi Roger,
On 3/06/2014 12:15 AM, roger riggs wrote:
Hi David, et.al.
I would let the compiler do auto-boxing where necessary. (Assuming object identity is not necessary).
I don't see where the compiler comes into this one. ???
David -----
If type disambiguation is necessary then use a cast to the target type and let the compiler do the rest. It keeps the source code simple and readable.
But I don't think it is worth a proactive pervasive change. The gain is overshadowed by the overhead of the reviews.
$.02, Roger
On 6/1/2014 11:07 PM, David Holmes wrote:
Hi Andrej,
Sorry for the delay getting back to you.
On 29/05/2014 10:24 PM, Andrej Golovnin wrote:
Hi David,
The valueOf calls may also allocate a new object so you can't just delete the JvmtiExport::post_vm_object_alloc call. Unfortunately you can't tell whether a new object was allocated or not. It is only for the smaller primitive types that any kind of Object caching is mandated.
It is only for the smaller values (-128 to +127) of the integer primitives types (plus boolean) that caching is mandated. Float.valueOf and Double.valueOf always create objects.
You are right, that #valueOf call may allocate an object. But as far as I understand currently the JvmtiExport::post_vm_object_alloc call is only needed, because today the native code itself allocates an object by calling java_lang_boxing_object::create(type, value, CHECK_NULL);.
Right, sorry - I was misunderstanding the purpose of the post_vm_object_alloc call:
http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#VMObjectAlloc
So from the perspective that you are diverting this back to Java code the hotspot changes look okay to me.
The more general question, for the core-libs folk, is whether changing everything to use valueOf is overkill (given the limits of the required caching mechanisms) or good to do from a consistency perspective. I'm slightly on the overkill side of things but not enough to reject things.
On the performance/benefit side, if I read things correctly you only see the 9GB of Boolean objects because you disable reflection-inflation - is that right? In that case, as Joel states, the gains are not really general, but on the other hand I don't see anything wrong with trying to improve the general efficiency here even if the greatest benefit comes from a "non-mainstream" usecase.
David -----
My code changes this behavior and delegates object allocation back to Java by calling
JavaCalls::call_static(&boxed_value, klass_handle, vmSymbols::valueOf_name(), valueOf_signature, &args, THREAD);
But maybe I misunderstood the implementation of JavaCalls.
Best regards, Andrej Golovnin
Hi, First, thanks for contributing this. Lets start with some process, have you signed the OCA? I have a lot of things on my plate right now so the progress of this might be slow, that doesn’t mean I have dropped it. I’m currently digesting the FieldAccessor mechanism, this will take some time. Comments inline, On 29 maj 2014, at 12:45, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:
Hi Joel,
When you have for example following method:
public int method() { return 0; }
And you invoke this method over the reflection API, then the first N invocations are done via the native code.
Yes, this is before inflation. Inflation happens after 15 calls IIRC, which is why I don’t think it matters from a performance standpoint.
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.
The bug entry says this is a “bug” but I disagree. There is nothing wrong with the current code, it just isn’t as efficient as it can be. IMO this is an enhancement, which influences trade-off like the HotSpot part. I won’t review the HotSpot change, and I don’t think it is that desirable given that we inflate after 15 calls, and it adds code to the VM.
Your tests show that the valueOf caches are used which is good. However the byte code spinning is a core piece of reflection that is currently in production in countless places. A change in this area should not only be obviously correct and thouroghly tested
That's why we do the code review. ;-)
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.
, you must show "real world” benefit. Ideally you should be able to show significant reduction in allocation in some application.
I'm working on a product which has ca. 3 million lines of code and make direct and indirect use of Reflection API. And in one of our use cases I see, that JVM allocates ca. 9GB of Boolean objects. All of them are allocated through the usage of Reflection API. Even that most of this objects are allocated in TLABs and are removed by GC when the use case is finished, I would say we have allocated 9GB of Boolean objects to much. Or do you see it differently?
No, this was exactly the kind of real world benefit I was looking for.
Let me know, if you need more data or if I should write some test.
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 where CONCURRENCY is optional. 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. cheers /Joel [1] https://wiki.openjdk.java.net/display/CodeTools/jcov
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
On 12 jun 2014, at 21:03, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:
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.
If jtreg is compiled with jcov support, the jtreg help has all the info you need. Unfortunately It looks like the jtreg binaries you can find doesn’t include jcov. The Adopt OpenJDK group compiles jtreg, perhaps you can work with them to get a build of jtreg + jcov? I’ll generate the coverage report myself, I’m curious if it works with the core reflection code, but it will probably happen faster if you do it. cheers /Joel
Hi Andrej, On 2014-06-12, Andrej Golovnin wrote:
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 don't want consistency here. I want the uninflated invocations/field gets to return new boxes for now.
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
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.
If you start a separate thread posting a reproducer I'll file bugs for this.
Before I start to change something I think we should make the decision whether we are going to drop the VM part or not.
I am dropping the VM part. Again, I have very limited time to work on this. We are looking into other changes in reflection that may or may not change and/or supersede this work. I think this would be a fine contribution to get in to 9, a good baseline to build further work upon, but the scope has to be limited. Given the risk-reward here my bar for accepting this is quite high. I'm looking forward to a rewritten test and some coverage information. cheers /Joel
Hi Joel, sorry for late response. I was too busy with other things. Again, I have very limited time to work on this. We are looking into
other changes in reflection that may or may not change and/or supersede this work.
Could you please share the details? I'm just curious.
I think this would be a fine contribution to get in to 9, a good baseline to build further work upon, but the scope has to be limited. Given the risk-reward here my bar for accepting this is quite high.
I'm looking forward to a rewritten test and some coverage information.
I have changed the test TestMethodReflectValueOf as you suggested and I have changed the summary of both tests too. You will find the changes in the attached patch. Here is the new webrev: https://db.tt/wQBLVceA And here is the coverage report in the HTML format: https://db.tt/JTZjpnMM Let me know if you need the coverage report in the text format. Best regards, Andrej Golovnin
Hi Andrej, On 22 jun 2014, at 00:00, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:
Hi Joel,
sorry for late response. I was too busy with other things.
Likewise!
I have changed the test TestMethodReflectValueOf as you suggested and I have changed the summary of both tests too. You will find the changes in the attached patch. Here is the new webrev: https://db.tt/wQBLVceA
And here is the coverage report in the HTML format:
Out of curiosity, did you generate the coverage report running the jdk_lang test suite? I think this patch is good to go. I need to file some Oracle internal requests, should take about a week, then I can sponsor this. Thanks for the contribution! cheers /Joel
Hi Joel,
I have changed the test TestMethodReflectValueOf as you suggested and I have changed the summary of both tests too. You will find the changes in the attached patch. Here is the new webrev: https://db.tt/wQBLVceA
And here is the coverage report in the HTML format:
Out of curiosity, did you generate the coverage report running the jdk_lang test suite?
Yes and I used the following config for JCov: include_list=jcov_jdk_lang.txt,field=on,abstract=on,native=on,type=all,file=$JCOV_OUT,merge=merge where jcov_jdk_lang.txt contains the single line: sun.reflect.*
I think this patch is good to go. I need to file some Oracle internal requests, should take about a week, then I can sponsor this.
I am very pleased to hear that and I hope to contribute more. Best regards, Andrej Golovnin
Hi Andrej, Can you resend the latest patch attached to a mail to this list? cheers /Joel On 2014-08-29, Andrej Golovnin wrote:
Hi Joel,
I have changed the test TestMethodReflectValueOf as you suggested and I have changed the summary of both tests too. You will find the changes in the attached patch. Here is the new webrev: https://db.tt/wQBLVceA
And here is the coverage report in the HTML format:
Out of curiosity, did you generate the coverage report running the jdk_lang test suite?
Yes and I used the following config for JCov:
include_list=jcov_jdk_lang.txt,field=on,abstract=on,native=on,type=all,file=$JCOV_OUT,merge=merge
where jcov_jdk_lang.txt contains the single line:
sun.reflect.*
I think this patch is good to go. I need to file some Oracle internal requests, should take about a week, then I can sponsor this.
I am very pleased to hear that and I hope to contribute more.
Best regards, Andrej Golovnin
--
Hi, I pushed this two days ago. Thank you Andrej for the contribution! cheers /Joel On 9 sep 2014, at 11:21, Joel Borggrén-Franck <joel.franck@oracle.com> wrote:
Hi Andrej,
Can you resend the latest patch attached to a mail to this list?
cheers /Joel
On 2014-08-29, Andrej Golovnin wrote:
Hi Joel,
I have changed the test TestMethodReflectValueOf as you suggested and I have changed the summary of both tests too. You will find the changes in the attached patch. Here is the new webrev: https://db.tt/wQBLVceA
And here is the coverage report in the HTML format:
Out of curiosity, did you generate the coverage report running the jdk_lang test suite?
Yes and I used the following config for JCov:
include_list=jcov_jdk_lang.txt,field=on,abstract=on,native=on,type=all,file=$JCOV_OUT,merge=merge
where jcov_jdk_lang.txt contains the single line:
sun.reflect.*
I think this patch is good to go. I need to file some Oracle internal requests, should take about a week, then I can sponsor this.
I am very pleased to hear that and I hope to contribute more.
Best regards, Andrej Golovnin
--
Hi, any update? Best regards, Andrej Golovnin On 29.05.2014, at 09:35, Joel Borggrén-Franck <joel.franck@oracle.com> wrote:
Hi,
We need you send in the patches via the mailing list. Please reply to your mail with the diffs attached directly (not zipped for example).
I’ll might have time to look at this next week, but it surprises me you need to patch hotspot. Allocations before we have inflated shouldn’t be a problem.
Also do you have any data showing that this actually makes a difference?
cheers /Joel
On 28 maj 2014, at 23:44, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:
Hi Joe,
I have prepared a patch for the issue JDK-5043030. The patch consists of two parts: one for jdk and one for hotspot. You can find the webrevs here:
JDK-patch: https://db.tt/7DYph0OH Hotspot-patch: https://db.tt/hHsN0yn4
The JDK-patch has two tests to verify the changes.
Please review it and I hope you can sponsor it.
Best regards, Andrej Golovnin
Hi all, as requested here are the patches as diffs. Best regards, Andrej Golovnin On 28.05.2014, at 23:44, Andrej Golovnin <andrej.golovnin@gmail.com> wrote:
Hi Joe,
I have prepared a patch for the issue JDK-5043030. The patch consists of two parts: one for jdk and one for hotspot. You can find the webrevs here:
JDK-patch: https://db.tt/7DYph0OH Hotspot-patch: https://db.tt/hHsN0yn4
The JDK-patch has two tests to verify the changes.
Please review it and I hope you can sponsor it.
Best regards, Andrej Golovnin
Hi all, as requested here are the diffs again as they were stripped last time. Best regards, Andrej Golovnin On Wed, May 28, 2014 at 11:44 PM, Andrej Golovnin <andrej.golovnin@gmail.com
wrote:
Hi Joe,
I have prepared a patch for the issue JDK-5043030. The patch consists of two parts: one for jdk and one for hotspot. You can find the webrevs here:
JDK-patch: https://db.tt/7DYph0OH Hotspot-patch: https://db.tt/hHsN0yn4
The JDK-patch has two tests to verify the changes.
Please review it and I hope you can sponsor it.
Best regards, Andrej Golovnin
participants (4)
-
Andrej Golovnin
-
David Holmes
-
Joel Borggrén-Franck
-
roger riggs