201893 - Too slow invocation of reflected methods for package-private classes

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[Igor Bukanov]

Steven Beal wrote to me some time ago:

-------- Original Message --------
Subject: Rhino performance tip (FAQ recommendation)
Date: Fri, 28 Feb 2003 10:20:34 -0800
From: Steven Beal <[e-mail]>
To: "Igor Bukanov (E-mail)" <[e-mail]>

Igor,

I was experiencing poor performance on the IBM virtual
machine (the one shipped with Websphere) and eventually
traced the problem to excessive throwing and catching
of exceptions.  

The Sun VM  handles exceptions much more efficiently
and thus does not suffer quite so much from exceptions
being thrown and caught in quick succession, which is
why I didn't notice the issue before.

This is not a bug in rhino, in fact the condition is
produced by a workaround for a Sun java bug

http://developer.java.sun.com/developer/bugParade/bugs/4071593.html

See org.mozilla.javascript.NativeJavaMethod.retryIllegalAccessInvoke().

The pattern used here is the same as that shown in one of the
annotations of the currently open incarnation of the same bug

http://developer.java.sun.com/developer/bugParade/bugs/4071957.html

This bug is not limited to inner classes though.  It
will also manifest itself why trying to invoke via reflection
a method on a pkg. private class offered via an Interface.

I place several references into the local scope created for
a request that are interface references to pkg. private classes
created via an Abstract Factory pattern.  By making these
classes public I avoided the need for the bug workaround and
improved performance significantly (a necessary compromise).

Perhaps some mention of this should be made in the FAQ since the
performance impact is significant (extremely so on the IBM VM).
The same problem would be seen when using Dynamic Proxies, 
and features of the Collection classes.  If you read through
the associated bug reports I'm sure you will find more cases.

Regards,

-Steve

Comment 1

[Igor Bukanov]

I think besides documentation it could be possible to do something with Rhino
code as well. If on the first failed method invocation code would search for a
public method in a class public super classes or interfaces which this method
overrides, then the issue can be solved.

Comment 2

[Igor Bukanov]

Attachment: Fix: after the first IllegalAccessException replace method by a better version if available.

The reason for performance problem is that the above
NativeJavaMethod.retryIllegalAccessInvoke() does not cache the accessible
method it found. Thus the idea is to replace on the first
IllegalAccessException the original method in NativeJavaMethod.methods by its
accessible version so the following invocations would not throw
IllegalAccessException.

It would be simple to modify retryIllegalAccessInvoke if only  NativeJavaMethod
used it, but the same problem presents with
JavaMembers.BeanProperty.setter/getter and I would like not to repeat the same
code there twice. To resolve this, I replaced Method references in BeanProperty
by references to NativeJavaMethod together with method indexes since previously
BeanProperty was created from NativeJavaMethod in any case. 

In this way single invoke utility in NativeJavaMethod would be sufficient and
if a getter method is called both directly and through a property access, it
will be replaced only one time. In addition, it allowed not to call
Method.getPrametersInfo() on setter during its invocation since this
information is already cached in NativeJavaMethod.

Comment 3

[Igor Bukanov]

Note for the above patch: it depends essentially on my chages to cache
Method.getParameterInfo() in NativeJavaMethod/NativeJavaConstructor that I put
to CVS on 2003-07-06.

Comment 4

[Igor Bukanov]

Attachment: Better approach: new class MemberBox to recover from IllegalArgumentException

The patch moved all logic to recover from IllegalArgumentException to new
MemberBox class that wraps Method or Constructor instances. The class also
cache results of getParameterTyps() and implements Serializable. The later
allows to skip implementing read/writeObject when class previously contained
Method or Constructor instances which are replaced by MemeberBox wrappers.

Comment 5

[Igor Bukanov]

I committed the patch. Steven does not have time to verify that now but perhaps
later he can test.

Comment 6

[Hannes Wallnoefer]

I think there are some problems with the patch you committed. I'm no longer able
to invoke public methods on non-public classes. The error message I get is:

TypeError: undefined is not a function.

Which may serve as clue, because it's not that we get a security exception when
invoking the method, the method is simply not there.

Also, when trying to invoke a public static method on a non-public class, I get
the following message:

org.mozilla.javascript.EvaluatorException: Java class "java.lang.Object" has no
public instance field or method named "foo".

Should I open a new bug for this, or will this one be reopened?

Comment 7

[Igor Bukanov]

Please open a new bug, since it is a different issue.

Comment 8

[Igor Bukanov]

I openned the bug 214608 about the problem reported in the comments 6

Comment 9

[Igor Bukanov]

Targeting as resolved against 1.5R5