Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve4 JavaMembers with cache and lazy load for improve the performance #619

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve4 JavaMembers with cache and lazy load for improve the performance #619

wants to merge 1 commit into from

Conversation

qxo
Copy link

@qxo qxo commented Nov 9, 2019

jmh benchmark: https://github.com/qxo/rhino-jmh-benchmark/blob/master/src/main/java/qxo/benchmark/rhino/RhinoJavaMembersBenchmark.java

rhino 1.7.11
====================================
Benchmark                                              Mode  Cnt   Score   Error   Units  Score/min
RhinoJavaMembersBenchmark.base4Member001p             thrpt   20  60.352 ± 1.670  ops/ms     14.026
RhinoJavaMembersBenchmark.base4Member010p             thrpt   20  59.010 ± 1.718  ops/ms     13.714
RhinoJavaMembersBenchmark.base4Member020p             thrpt   20  56.018 ± 1.780  ops/ms     13.019
RhinoJavaMembersBenchmark.base4Member050p             thrpt   20  56.259 ± 2.028  ops/ms     13.075
RhinoJavaMembersBenchmark.base4Member100p             thrpt   20  56.998 ± 1.300  ops/ms     13.247
RhinoJavaMembersBenchmark.base4Member200p             thrpt   20  54.377 ± 2.047  ops/ms     12.637
RhinoJavaMembersBenchmark.lazyInitOff4Member050p      thrpt   20  21.301 ± 1.565  ops/ms      4.950
RhinoJavaMembersBenchmark.lazyInitOn4Member050p       thrpt   20  57.212 ± 1.699  ops/ms     13.296
RhinoJavaMembersBenchmark.old4Member001p              thrpt   20  24.235 ± 0.553  ops/ms      5.632
RhinoJavaMembersBenchmark.old4Member050p              thrpt   20  11.430 ± 0.163  ops/ms      2.656
RhinoJavaMembersBenchmark.old4Member100p              thrpt   20   7.230 ± 0.048  ops/ms      1.680
RhinoJavaMembersBenchmark.old4Member200p              thrpt   20   4.303 ± 0.038  ops/ms      1.000
RhinoJavaMembersBenchmark.reflectCacheOff4Member050p  thrpt   20  14.271 ± 1.371  ops/ms      3.317


rhino  1.7.7.2
==================================
Benchmark                                              Mode  Cnt   Score   Error   Units  Score/min
RhinoJavaMembersBenchmark.base4Member001p             thrpt   20  58.649 ± 0.272  ops/ms     13.180
RhinoJavaMembersBenchmark.base4Member010p             thrpt   20  57.837 ± 0.323  ops/ms     12.998
RhinoJavaMembersBenchmark.base4Member020p             thrpt   20  57.664 ± 0.216  ops/ms     12.959
RhinoJavaMembersBenchmark.base4Member050p             thrpt   20  57.913 ± 0.168  ops/ms     13.015
RhinoJavaMembersBenchmark.base4Member100p             thrpt   20  57.923 ± 0.605  ops/ms     13.017
RhinoJavaMembersBenchmark.base4Member200p             thrpt   20  57.063 ± 0.784  ops/ms     12.824
RhinoJavaMembersBenchmark.lazyInitOff4Member050p      thrpt   20  25.693 ± 0.381  ops/ms      5.774
RhinoJavaMembersBenchmark.lazyInitOn4Member050p       thrpt   20  56.958 ± 1.618  ops/ms     12.800
RhinoJavaMembersBenchmark.old4Member001p              thrpt   20  26.867 ± 0.202  ops/ms      6.038
RhinoJavaMembersBenchmark.old4Member050p              thrpt   20  12.091 ± 0.139  ops/ms      2.717
RhinoJavaMembersBenchmark.old4Member100p              thrpt   20   7.588 ± 0.077  ops/ms      1.705
RhinoJavaMembersBenchmark.old4Member200p              thrpt   20   4.450 ± 0.026  ops/ms      1.000
RhinoJavaMembersBenchmark.reflectCacheOff4Member050p  thrpt   20  15.193 ± 0.148  ops/ms      3.414

@gbrail gbrail self-requested a review November 11, 2019 23:24
gbrail
gbrail requested changes Nov 11, 2019
View reviewed changes
Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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!

  2. I have a number of stylistic comments, which I attached to this review.

  3. 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!

src/org/mozilla/javascript/JavaMembers.java Outdated
@@ -918,3 +740,988 @@ public Object getDefaultValue(Class<?> hint)
Field field;
Object javaObject;
}

class JavaMembersNew extends JavaMembers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

src/org/mozilla/javascript/JavaMembers.java Outdated

class JavaMembersNew extends JavaMembers
{
JavaMembersNew(Scriptable scope, Class<?> cl)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

src/org/mozilla/javascript/JavaMembers.java Outdated
// Try to get static member from instance (LC3)
member = getMember(scope, name, true);
if( member == null) {
final Map<String,Object> ht = isStatic ? staticMembers : members;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, "gradlew check" is failing on this line because "isStatic" is always false.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

src/org/mozilla/javascript/JavaMembers.java Outdated
boolean includeProtected,
boolean includePrivate)
{
// We reflect methods first, because we want overloaded field/method
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Author

@qxo qxo Nov 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

src/org/mozilla/javascript/JavaMembers.java Outdated
private MemberBox findGetter(boolean isStatic, Map<String,Object> ht, String prefix,
String propertyName)
{
String getterName = prefix.concat(propertyName);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indenting.

src/org/mozilla/javascript/JavaMembers.java Outdated
// names to be allocated to the NativeJavaMethod before the field
// gets in the way.

Method[] methods = discoverAccessibleMethods(cl, includeProtected,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indenting.

src/org/mozilla/javascript/JavaMembers.java Outdated
}

//rhino_JavaMembers_reflect_cache_on=false for disable
private static final boolean CACHE_ON = !"false".equals(getProperty("rhino_JavaMembers_reflect_cache_on","true"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:)

@qxo
Copy link
Author

qxo commented Nov 12, 2019

@gbrail sorry, still busy, I'll check your review if I have the time
Or you change the code by yourself:)

@p-bakker p-bakker added Java Interop Issues related to the interaction between Java and JavaScript Performance Issues related to the performance of the Rhino engine labels Jun 30, 2021
@p-bakker
Copy link
Collaborator

@qxo is been 2 years since the last activity on this.

Is this still something you'd like to get merged?

@qxo
Copy link
Author

qxo commented Jan 20, 2022

@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 qxo force-pushed the improve4JavaMembers branch 3 times, most recently from 3f921a0 to 802724c Compare January 20, 2022 11:56
@p-bakker
Copy link
Collaborator

@qxo I see you pushed a commit. Is that just a rebase of the earlier changes or does it also address the comments made by @gbrail?

@qxo
Copy link
Author

qxo commented Jan 21, 2022 •
edited by p-bakker
Loading

@qxo I see you pushed a commit. Is that just a rebase of the earlier changes or does it also address the comments made by @gbrail?

Yes, it address the comments made by @gbrail.
But it's not just a rebase. It's a remerge (git cherry-pick then merge) and fix the code style and bug that found by gradle check

@gbrail
Copy link
Collaborator

gbrail commented Jan 27, 2022

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?

@p-bakker
Copy link
Collaborator

@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 qxo force-pushed the improve4JavaMembers branch 2 times, most recently from fa58dbb to f4060e7 Compare February 15, 2022 13:31
@p-bakker p-bakker requested a review from gbrail February 15, 2022 13:34
@p-bakker
Copy link
Collaborator

Tnx @qxo

@gbrail think the has open review comment has been addressed by the last commits (#619 (comment))

@qxo
Copy link
Author

qxo commented Feb 15, 2022

@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

Yes, I prefer to use SPI or system properties:
with SPI, the programmer can change the config store without change the sourcecode.
with system properties we can change feature flag out of box.
but with feature flags, there is no way change it without change the program :(

@p-bakker
Copy link
Collaborator

Cant roll your own ContextFactory where you enable/disable features based on values of system properties?

@qxo
Copy link
Author

qxo commented Feb 16, 2022

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.

@p-bakker p-bakker mentioned this pull request Mar 7, 2022
Draft
@p-bakker
Copy link
Collaborator

p-bakker commented Sep 8, 2024

@gbrail looks like all review comments are addressed, agree? If so, this just needs a rebase to be compatible with the latest source

@gbrail
Copy link
Collaborator

gbrail commented Sep 11, 2024

Well we told the author to "be patient" in 2019...

But seriously, this is not a simple merge -- I just tried to just resolve the merge conflicts and I can already see that that is not enough, that some deeper thinking is needed, so it'll be a bigger job for someone.

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 11, 2024 •
edited
Loading

@gbrail is your pending change request done though? If so, can you resolve it?

As for the 'Deeper thinking required': you mean wrt properly rebasing it or wrt to the problem this PR is trying to solve or the direction that was taken to shove the problem?

@gbrail
Copy link
Collaborator

gbrail commented Sep 12, 2024 via email

@p-bakker
Copy link
Collaborator

@qxo could we maybe convince you to rebase your PR once again?

@qxo
Copy link
Author

qxo commented Sep 12, 2024 via email

@p-bakker
Copy link
Collaborator

@qxo thx! And yes, its been so long. Being all volunteers here, attention comes and goes I'm afraid, but lately we've been on a roll 🙂

@qxo
Copy link
Author

qxo commented Sep 16, 2024

@qxo thx! And yes, its been so long. Being all volunteers here, attention comes and goes I'm afraid, but lately we've been on a roll 🙂

I remerged this feature to the master branch, compile ok, but not check testcases

@p-bakker
Copy link
Collaborator

Pipeline fails due to formatting issues

As the logs says:

Run './gradlew :rhino:spotlessApply' to fix these violations

@gbrail
Copy link
Collaborator

gbrail commented Sep 16, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbrail gbrail Awaiting requested review from gbrail

Assignees
No one assigned
Labels
Java Interop Issues related to the interaction between Java and JavaScript Performance Issues related to the performance of the Rhino engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qxo @p-bakker @gbrail