RFR: 5043030 (reflect) unnecessary object creation in reflection

[Joel Borggrén-Franck]

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 at 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