Improve4 JavaMembers with cache and lazy load for improve the performance #619
Conversation
There was a problem hiding this comment.
Thanks for doing this! I'm in favor of performance improvements.
Please be patient -- it's going to take a little while to work through this since it looks like a big change. I have suggestions in three areas:
-
Right now, "gradlew check" fails. First, because there is some unreferenced code that SpotBugs finds. Second, because some tests are failing. Please run "gradlew check" and take a look!
-
I have a number of stylistic comments, which I attached to this review.
-
I'd like to understand the caching behavior better. In particular, can we document somewhere (like in a comment on this class) if the cache needs to be invalidated sometimes, or not, and if there's a chance that it will grow and grow forever in some contexts, or if there's some way to limit the size. (That can help us decide whether this cache should be enabled by default.)
Thanks!
| @@ -918,3 +740,988 @@ public Object getDefaultValue(Class<?> hint) | |||
| Field field; | |||
| Object javaObject; | |||
| } | |||
|
|
|||
| class JavaMembersNew extends JavaMembers | |||
There was a problem hiding this comment.
There is code in this (very old) codebase that has more than one top-level Java class per .java source file, but that's not a great pattern and something that Java developers rarely do any more. I would prefer if the various classes in this file are either all nested classes (they can be "static" if they don't need access to the parent) or if they were in different source files. (And since this change basically looks like a total rewrite, it might be easier to see what changed.)
|
|
||
| class JavaMembersNew extends JavaMembers | ||
| { | ||
| JavaMembersNew(Scriptable scope, Class<?> cl) |
There was a problem hiding this comment.
Also, class names like "JavaMembersNew" and "JavaMembersOld" aren't necessarily easy to understand. Is one class intended to be "with cache" and the other "without cache?" It might make it easier to call that out in the name.
There was a problem hiding this comment.
Also, class names like "JavaMembersNew" and "JavaMembersOld" aren't necessarily easy to understand. Is one class intended to be "with cache" and the other "without cache?" It might make it easier to call that out in the name.
I perfer "JavaMembersNew" to "JavaMembers4CacheAndLazyWay" and "JavaMembersOld" to "JavaMembers4NoCacheNoLazy", is that ok ?
| // Try to get static member from instance (LC3) | ||
| member = getMember(scope, name, true); | ||
| if( member == null) { | ||
| final Map<String,Object> ht = isStatic ? staticMembers : members; |
There was a problem hiding this comment.
FWIW, "gradlew check" is failing on this line because "isStatic" is always false.
There was a problem hiding this comment.
FWIW, "gradlew check" is failing on this line because "isStatic" is always false.
How to find the "gradlew check" detail report, I am unfamilar for the "gradlew check"
| boolean includeProtected, | ||
| boolean includePrivate) | ||
| { | ||
| // We reflect methods first, because we want overloaded field/method |
There was a problem hiding this comment.
Looks like we stopped indenting here. The main rule we have in this codebase right now is to keep the code style consistent with whatever else is already in this file, so could you please fix the indenting here and elsewhere?
(I wish that we had strict indenting standards for this project but we're about 20 years too late.)
There was a problem hiding this comment.
Looks like we stopped indenting here. The main rule we have in this codebase right now is to keep the code style consistent with whatever else is already in this file, so could you please fix the indenting here and elsewhere?
(I wish that we had strict indenting standards for this project but we're about 20 years too late.)
I agree , I'll fix later:)
we can do better, why not.
And is there any existed code style I can follow? ( My IDE was eclipse )
| private MemberBox findGetter(boolean isStatic, Map<String,Object> ht, String prefix, | ||
| String propertyName) | ||
| { | ||
| String getterName = prefix.concat(propertyName); |
| // names to be allocated to the NativeJavaMethod before the field | ||
| // gets in the way. | ||
|
|
||
| Method[] methods = discoverAccessibleMethods(cl, includeProtected, |
| } | ||
|
|
||
| //rhino_JavaMembers_reflect_cache_on=false for disable | ||
| private static final boolean CACHE_ON = !"false".equals(getProperty("rhino_JavaMembers_reflect_cache_on","true")); |
There was a problem hiding this comment.
I would rather do this kind of optional stuff by following the existing pattern, which uses constants in the Context class and code in ContextFactory to select this feature. But I am glad that you have thought of making it optional!
There was a problem hiding this comment.
In my own project I perfer SPI(java.util.ServiceLoader) way for the configuration
I would rather do this kind of optional stuff by following the existing pattern, which uses constants in the Context class and code in ContextFactory to select this feature. But I am glad that you have thought of making it optional!
In my own project I perfer SPI(java.util.ServiceLoader) way for the configuration, so that the app can do the configuration it's own way:)
|
@gbrail sorry, still busy, I'll check your review if I have the time |
p-bakker
added
Java Interop
|
@qxo is been 2 years since the last activity on this. Is this still something you'd like to get merged? |
Sorry for the delay, I'll try to remerge recently:) |
qxo
force-pushed
the
improve4JavaMembers
branch
2 times, most recently
from
659e11f
to
3f921a0
Compare
rhino_JavaMembers_lazyInit=false rhino_JavaMembers_reflect_cache_on=true rhino_JavaMembers_lazyInit=true rhino_JavaMembers_reflect_cache_on=false
qxo
force-pushed
the
improve4JavaMembers
branch
from
3f921a0
to
802724c
Compare
Yes, it address the comments made by @gbrail. |
|
This works for me, passes all the tests, and I'm almost ready to merge it. I do think it'd be more consistent to add feature flags to the Context class instead of (or in addition to) the system properties, for consistency with the rest of Rhino. Would you be willing to do that? |
|
@qxo saw @gbrail last comment, about adding feature flags to Context instead of system properties? I saw you prefer to use SPI in your projects, but in Rhino all (or should I say nearly all) configuration is done through the Context class, so I think to stay consistent, your caching options should follow tha same approach |
qxo
force-pushed
the
improve4JavaMembers
branch
from
4de4536
to
fa58dbb
Compare
qxo
force-pushed
the
improve4JavaMembers
branch
from
fa58dbb
to
f4060e7
Compare
|
Tnx @qxo @gbrail think the has open review comment has been addressed by the last commits (#619 (comment)) |
Yes, I prefer to use SPI or system properties: |
|
Cant roll your own ContextFactory where you enable/disable features based on values of system properties? |
Yes. But we have to do some hack if the code used by 3rd party library. |
jmh benchmark: https://github.com/qxo/rhino-jmh-benchmark/blob/master/src/main/java/qxo/benchmark/rhino/RhinoJavaMembersBenchmark.java