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.

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

Consolidate multiple optimization levels down to one #1754

Merged
merged 5 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ public static class AbstractClassState {
public void init()
throws IllegalAccessException, InvocationTargetException, InstantiationException {
cx = Context.enter();
cx.setOptimizationLevel(9);
cx.setLanguageVersion(Context.VERSION_ES6);

scope = cx.initStandardObjects();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ public static class MathState {
@Setup(Level.Trial)
public void setup() throws IOException {
cx = Context.enter();
cx.setOptimizationLevel(9);
cx.setLanguageVersion(Context.VERSION_ES6);
scope = cx.initStandardObjects();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ public static class FieldTestState {
@SuppressWarnings("unused")
public void create() throws IOException {
cx = Context.enter();
cx.setOptimizationLevel(9);
cx.setLanguageVersion(Context.VERSION_ES6);

scope = new Global(cx);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ public static class PropertyState {
@Setup(Level.Trial)
public void setup() throws IOException {
cx = Context.enter();
cx.setOptimizationLevel(9);
cx.setLanguageVersion(Context.VERSION_ES6);
scope = cx.initStandardObjects();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ abstract static class AbstractState {
public void setUp() {
cx = Context.enter();
cx.setLanguageVersion(Context.VERSION_ES6);
cx.setOptimizationLevel(9);
scope = cx.initStandardObjects();

try (FileReader rdr = new FileReader(fileName)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ void evaluateSource(Context cx, Scriptable scope, String fileName) {
void initialize() {
cx = Context.enter();
cx.setLanguageVersion(Context.VERSION_ES6);
cx.setOptimizationLevel(9);
scope = cx.initStandardObjects();
evaluateSource(cx, scope, "testsrc/benchmarks/framework.js");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,18 @@
public class RhinoScriptEngine extends AbstractScriptEngine implements Compilable, Invocable {

/**
* Reserved key for the Rhino optimization level. Default is "9," for optimized and compiled
* code. Set this to "-1" to run Rhino in interpreted mode -- this is much much slower but the
* only option on platforms like Android that don't support class files.
* Reserved key for the Rhino optimization level. This is supported for backward compatibility
* -- any value less than zero results in using interpreted mode.
*/
public static final String OPTIMIZATION_LEVEL = "org.mozilla.javascript.optimization_level";

/**
* Reserved key for interpreted mode, which is much slower than the default compiled mode but
* necessary on Android where Rhino can't generate class files.
*/
public static final String INTERPRETED_MODE = "org.mozilla.javascript.interpreted_mode";

static final int DEFAULT_LANGUAGE_VERSION = Context.VERSION_ES6;
private static final int DEFAULT_OPT = 9;
private static final boolean DEFAULT_DEBUG = true;
private static final String DEFAULT_FILENAME = "eval";

Expand Down Expand Up @@ -275,7 +279,14 @@ private void configureContext(Context cx) throws ScriptException {
}
Object ol = get(OPTIMIZATION_LEVEL);
if (ol != null) {
cx.setOptimizationLevel(parseInteger(ol));
int lvl = parseInteger(ol);
if (lvl < 0) {
cx.setInterpretedMode(true);
}
}
Object interpreted = get(INTERPRETED_MODE);
if (interpreted != null) {
cx.setInterpretedMode(parseBoolean(interpreted));
}
}

Expand All @@ -287,12 +298,22 @@ private static int parseInteger(Object v) throws ScriptException {
throw new ScriptException("Invalid number " + v);
}
} else if (v instanceof Integer) {
return ((Integer) v).intValue();
return (Integer) v;
} else {
throw new ScriptException("Value must be a string or number");
}
}

private static boolean parseBoolean(Object v) throws ScriptException {
if (v instanceof String) {
return Boolean.parseBoolean((String) v);
} else if (v instanceof Boolean) {
gbrail marked this conversation as resolved.
Show resolved Hide resolved
return (Boolean) v;
} else {
gbrail marked this conversation as resolved.
Show resolved Hide resolved
throw new ScriptException("Value must be a string or boolean");
}
}

private String getFilename() {
Object fn = get(ScriptEngine.FILENAME);
if (fn instanceof String) {
Expand Down Expand Up @@ -327,7 +348,6 @@ protected boolean hasFeature(Context cx, int featureIndex) {
@Override
protected void onContextCreated(Context cx) {
cx.setLanguageVersion(Context.VERSION_ES6);
cx.setOptimizationLevel(DEFAULT_OPT);
cx.setGeneratingDebug(DEFAULT_DEBUG);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -738,10 +738,10 @@ private static String do_eval(Context cx, StackFrame frame, String expr) {
String resultString;
Debugger saved_debugger = cx.getDebugger();
Object saved_data = cx.getDebuggerContextData();
int saved_level = cx.getOptimizationLevel();
boolean wasInterpreted = cx.isInterpretedMode();

cx.setDebugger(null, null);
cx.setOptimizationLevel(-1);
cx.setInterpretedMode(false);
cx.setGeneratingDebug(false);
try {
Callable script = (Callable) cx.compileString(expr, "", 0, null);
Expand All @@ -755,7 +755,7 @@ private static String do_eval(Context cx, StackFrame frame, String expr) {
resultString = exc.getMessage();
} finally {
cx.setGeneratingDebug(true);
cx.setOptimizationLevel(saved_level);
cx.setInterpretedMode(wasInterpreted);
cx.setDebugger(saved_debugger, saved_data);
}
if (resultString == null) {
Expand Down Expand Up @@ -874,7 +874,7 @@ public void contextCreated(Context cx) {
Debugger debugger = new DimIProxy(dim, IPROXY_DEBUG);
cx.setDebugger(debugger, contextData);
cx.setGeneratingDebug(true);
cx.setOptimizationLevel(-1);
cx.setInterpretedMode(true);
}

/** Called when a Context is destroyed. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,7 @@ public String[] processOptions(String args[]) {
continue;
}
if ((arg.equals("-opt") || arg.equals("-O")) && ++i < args.length) {
int optLevel = Integer.parseInt(args[i]);
compilerEnv.setOptimizationLevel(optLevel);
// This no longer has an effect, but parse it for backward compatibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can add a version hint to the comment, i fear when reading this later someone likes to know the version that introduces this change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Check out new comments, thanks!

continue;
}
} catch (NumberFormatException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ public static String[] processOptions(String args[]) {
continue;
}
if (arg.equals("-opt") || arg.equals("-O")) {
// This bit remains for backward compatibility
if (++i == args.length) {
usageError = arg;
break goodUsage;
Expand All @@ -296,14 +297,13 @@ public static String[] processOptions(String args[]) {
usageError = args[i];
break goodUsage;
}
if (opt == -2) {
// Compatibility with Cocoon Rhino fork
opt = -1;
} else if (!Context.isValidOptimizationLevel(opt)) {
usageError = args[i];
break goodUsage;
if (opt < 0) {
shellContextFactory.setInterpretedMode(true);
}
shellContextFactory.setOptimizationLevel(opt);
continue;
}
if (arg.equals("-int")) {
shellContextFactory.setInterpretedMode(true);
continue;
}
if (arg.equals("-encoding")) {
Expand Down Expand Up @@ -551,7 +551,7 @@ static void processFileSecure(Context cx, Scriptable scope, String path, Object
Object source = readFileOrUrl(path, !isClass);

byte[] digest = getDigest(source);
String key = path + "_" + cx.getOptimizationLevel();
String key = path + "_" + (cx.isInterpretedMode() ? "int" : "comp");
Copy link
Collaborator

Choose a reason for hiding this comment

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

'int' might be confusing, is there any good reason for this shortening?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really -- AFAIK this is just used as a cache key but there's no reason why not to make it longer. (Previously it was just an integer.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checkout the latest patch, it supports both "-int" and "-interpreted"

ScriptReference ref = scriptCache.get(key, digest);
Script script = ref != null ? ref.get() : null;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public class ShellContextFactory extends ContextFactory {
private boolean strictMode;
private boolean warningAsError;
private int languageVersion = Context.VERSION_ES6;
private int optimizationLevel;
private boolean interpretedMode;
private boolean generatingDebug;
private boolean allowReservedKeywords = true;
private ErrorReporter errorReporter;
Expand Down Expand Up @@ -43,7 +43,7 @@ protected boolean hasFeature(Context cx, int featureIndex) {
@Override
protected void onContextCreated(Context cx) {
cx.setLanguageVersion(languageVersion);
cx.setOptimizationLevel(optimizationLevel);
cx.setInterpretedMode(interpretedMode);
if (errorReporter != null) {
cx.setErrorReporter(errorReporter);
}
Expand All @@ -67,10 +67,9 @@ public void setLanguageVersion(int version) {
this.languageVersion = version;
}

public void setOptimizationLevel(int optimizationLevel) {
Context.checkOptimizationLevel(optimizationLevel);
public void setInterpretedMode(boolean interpreted) {
checkNotSealed();
this.optimizationLevel = optimizationLevel;
this.interpretedMode = interpreted;
}

public void setErrorReporter(ErrorReporter errorReporter) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ msg.shell.usage =\
\ -w Enable warnings.\n\
\ -version 100|110|120|130|140|150|160|170|180|200\n\
\ Set a specific language version.\n\
\ -opt [-1|0-9] Set optimization level.\n\
\ -int Run in interpreted mode.\n\
Copy link
Collaborator

Choose a reason for hiding this comment

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

'int' is confusing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, although don't you think that '-interpreted' is too long? And '-I' is too much like the "-I" argument in a C compiler.

What if we support both "-int" and "-interpreted" command-line flags?

Copy link
Collaborator

Choose a reason for hiding this comment

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

although don't you think that '-interpreted' is too long?

sometimes, but i learned the hard way that is easier for me to type this many chars while writing the code & doc the spending the the answering all the questions ;-D

\ -f script-filename Execute script file, or "-" for interactive.\n\
\ -e script-source Evaluate inline script.\n\
\ -modules [uri] Add a single path or URL element to the CommonJS\n\
Expand Down Expand Up @@ -114,8 +114,6 @@ Usage: java {0} [OPTION]... SOURCE...\n\
Valid options are: \n\
\ -version VERSION Use the specified language version.\n\
\ VERSION should be one of 100|110|120|130|140|150|160|170.\n\
\ -opt LEVEL Use optimization with the specified level.\n\
\ LEVEL should be one of 0..9.\n\
\ -debug, -g Include debug information.\n\
\ -nosource Do not include source to function objects.\n\
\ It makes f.toString() useless and violates ECMAScript\n\
Expand Down
20 changes: 15 additions & 5 deletions rhino/src/main/java/org/mozilla/javascript/CompilerEnvirons.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public CompilerEnvirons() {
reservedKeywordAsIdentifier = true;
allowMemberExprAsFunctionName = false;
xmlAvailable = true;
optimizationLevel = 0;
interpretedMode = false;
generatingSource = true;
strictMode = false;
warningAsError = false;
Expand All @@ -35,7 +35,7 @@ public void initFromContext(Context cx) {
warningAsError = cx.hasFeature(Context.FEATURE_WARNING_AS_ERROR);
xmlAvailable = cx.hasFeature(Context.FEATURE_E4X);

optimizationLevel = cx.getOptimizationLevel();
interpretedMode = cx.isInterpretedMode();

generatingSource = cx.isGeneratingSource();
activationNames = cx.activationNames;
Expand Down Expand Up @@ -98,13 +98,23 @@ public void setXmlAvailable(boolean flag) {
xmlAvailable = flag;
}

@Deprecated
gbrail marked this conversation as resolved.
Show resolved Hide resolved
public final int getOptimizationLevel() {
return optimizationLevel;
return interpretedMode ? -1 : 9;
}

public final boolean isInterpretedMode() {
return interpretedMode;
}

@Deprecated
public void setOptimizationLevel(int level) {
Context.checkOptimizationLevel(level);
this.optimizationLevel = level;
interpretedMode = (level < 0);
}

public void setInterpretedMode(boolean interpretedMode) {
this.interpretedMode = interpretedMode;
}

public final boolean isGeneratingSource() {
Expand Down Expand Up @@ -255,7 +265,7 @@ public static CompilerEnvirons ideEnvirons() {
private boolean reservedKeywordAsIdentifier;
private boolean allowMemberExprAsFunctionName;
private boolean xmlAvailable;
private int optimizationLevel;
private boolean interpretedMode;
private boolean generatingSource;
private boolean strictMode;
private boolean warningAsError;
Expand Down
Loading
Loading