Skip to content

Introduce EvaluationMethod enum to enable the introduction of new interpreter.#2424

Merged
gbrail merged 3 commits into
mozilla:masterfrom
aardvark179:aardvark179-evaluation-mode
Jun 9, 2026
Merged

Introduce EvaluationMethod enum to enable the introduction of new interpreter.#2424
gbrail merged 3 commits into
mozilla:masterfrom
aardvark179:aardvark179-evaluation-mode

Conversation

@aardvark179

Copy link
Copy Markdown
Contributor

This change introduces an enum that expresses the possible methods by which code can be evaluated and encapsulates the logic to instantiate interpreters or compilers for that method.

Co-authored-by: Jimmy Miller <jimmy.miller@servicenow.com>
Co-authored-by: Cam Walter <cam.n.walter@gmail.com>
@aardvark179
aardvark179 force-pushed the aardvark179-evaluation-mode branch 6 times, most recently from 50f9677 to 286e464 Compare June 3, 2026 17:17
@gbrail

gbrail commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Is the idea here that there are multiple options going forward other than two? If so then fine, it's just an intrusive change to an interface we just changed recently.

@aardvark179
aardvark179 force-pushed the aardvark179-evaluation-mode branch from 286e464 to 5840cf5 Compare June 4, 2026 15:00
@aardvark179

Copy link
Copy Markdown
Contributor Author

Is the idea here that there are multiple options going forward other than two? If so then fine, it's just an intrusive change to an interface we just changed recently.

Exactly that, this is the second thing on the list for #2416. Now I've got a branch with everything laid out somewhat sensibly I'll create a draft PR for the whole thing with an explanation of the architecture and why we did it.

@aardvark179

Copy link
Copy Markdown
Contributor Author

I should point out I am not getting rid of the setInterpreterMode or the setOptimizationLevel APIs. The reason for introducing an enum and changing the tests to use this this is that the new interpreter can be introduced without touching the tests themselves.

Comment thread benchmarks/build.gradle Outdated
}
}
jvmArgs = ['-XX:+BackgroundCompilation']
jvmArgs = ['-XX:-BackgroundCompilation']

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As we discussed in our call, this should really be moved to its own PR

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, please take a look at this so we don't leak too much stuff together here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed. I'll make a separate PR for this.

try {
cx.setInterpretedMode(true); // must use interpreter mode
cx.setEvaluationMethod(EvaluationMethod.Interpreter);
; // must use interpreter mode

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These semicolons on their own seem to be an accident

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I thought I'd cleaned that up, but apparently I completely failed to actually push stuff.

@andreabergia

Copy link
Copy Markdown
Contributor

I should point out I am not getting rid of the setInterpreterMode or the setOptimizationLevel APIs. The reason for introducing an enum and changing the tests to use this this is that the new interpreter can be introduced without touching the tests themselves.

Why not?
Since 2.0 requires anyway various changes from any user, could we take the opportunity to completely drop these API? I think at least the setInterpreterMode has been deprecated for a while?

@gbrail

gbrail commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

as for changing "setInterpreterMode," we only changed this interface very recently in Rhino timescale (earlier this year if I recall, or late last year), so we should be careful. Maintaining those few old methods for backward compatibility seems pretty harmless to me at the moment.

aardvark179 and others added 2 commits June 8, 2026 18:57
Co-authored-by: Jimmy Miller <jimmy.miller@servicenow.com>
Co-authored-by: Cam Walter <cam.n.walter@gmail.com>
Co-authored-by: Jimmy Miller <jimmy.miller@servicenow.com>
Co-authored-by: Cam Walter <cam.n.walter@gmail.com>
@aardvark179
aardvark179 force-pushed the aardvark179-evaluation-mode branch from 5840cf5 to e38377e Compare June 8, 2026 18:02
@gbrail

gbrail commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

This looks good now, thanks!

@gbrail
gbrail merged commit b78871b into mozilla:master Jun 9, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants