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.golov...@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
