-
Notifications
You must be signed in to change notification settings - Fork 903
Replace addScriptRuntimeInvoke("string-literal") with method info computed at runtime
#1950
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
I haven't had a chance to read this yet, but you can check here for the warning: |
|
This is really cool. But please take a look if this will work when method with different signatures are here as in this PR https://github.com/mozilla/rhino/pull/1957/files |
Yes it will work. Take rhino/rhino/src/main/java/org/mozilla/javascript/optimizer/ScriptRuntimeMethodSig.java Lines 50 to 53 in e9e5255
There're 4 |
| ScriptRuntime.class.getName().replace('.', '/'); | ||
|
|
||
| static { | ||
| // init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This init code is really "java-magic"
- the ScriptRuntime-class needs the MethodSig-enum in annotations
- the static initialzer of the MethodSig-enum needs the annotations in ScriptRuntime.
First I thought, that this will not work, because we have a cycle (but somehow the JVM can handle this and probably in some JLS the behavior is described exactly)
if we are sure, that this will work on all VMs etc, all good
...otherwise I would suggest to move the init code to the BodyCodeGen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I briefly walked through JLS for enum class, and unfortunately there doesn't seem to be a behavior definition for what we're doing here. But I do found out that:
enum SomeEnumClass {
static {
SomeEnumClass.values();
}
}is valid, which indicates that enum members are already initialized when doing class static initialization, so what we're doing here should be fine
|
This is a clever solution, barring @rbri 's comments about static initialization order. Is it your intention to see this through to the end? The bootstrap method for INDY instructions, and everything in OptRuntime, for example, still aren't replaced using this mechanism. |
My plan is to, at least, replace all method references in |
The intention of this PR is the same as #1948 : to make code gen works even after a package renaming (aka relocating)
In order to handle such a large amount of method references to
ScriptRuntime, a new enum classScriptRuntimeMethodSigis introduced, with each enum value holding code gen related information for a method inScriptRuntime.See javadoc for
ScriptRuntimeMethodSigfor more info: linkIn theory this PR is completed and ready for review, but I dont know how to suppress the "ImmutableEnum" error when doing
gradle check, so draft for now