Closed
Bug 908903
Opened 11 years ago
Closed 11 years ago
Add a shell function for switching compiler options at runtime.
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #902690 +++
So far I have been unable to reproduce any bug with the patch provided as part of Bug 902690. So I am opening this bug to extract the part of the patch which deals with the extra JS Shell function and use it as a generic toggle for some Compiler options, which can be switched at runtime.
The first two that I am looking into right now are the baseline & ion use count. As followup bugs/patches/ we could also add other options such as the Call & OSI point checks.
This way we can prevent adding many command line flags to the test suite.
Comment 1•11 years ago
|
||
Another option might be to do bug 880330 and add the JIT flags to the options argument to `newGlobal`.
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #1)
> Another option might be to do bug 880330 and add the JIT flags to the
> options argument to `newGlobal`.
Compiler options are not attached to any runtime / context or any. So it does not make any sense to have it part of the newGlobal function. On the other hand, we can handle JSContext specific options with this interface.
Note that the JS_OPTION are different than the compiler option that I mention in comment 0. The Compiler options are contained in jit/Ion.h (see IonOptions).
I already made a JSAPI function to use these for fuzzing, such as Jesse can test ion-eager flags in the browser, and I am just reusing the same API to add this JS Shell function.
Assignee | ||
Comment 3•11 years ago
|
||
This patch introduces a new function to the JS shell. This function is available in both debug and optimized build. This function can be used as follow:
- Switch all future compilation to be --baseline-eager & --ion-eager compilations
setCompilerOption("baseline.usecount.trigger", 0);
setCompilerOption("ion.usecount.trigger", 0);
- Trigger Baseline / Ion compilations after 2 / 5 iterations.
setCompilerOption("baseline.usecount.trigger", 2);
setCompilerOption("ion.usecount.trigger", 5);
- Reset default triggers (ignores the command line):
setCompilerOption("baseline.usecount.trigger", -1); // reset to 10
setCompilerOption("ion.usecount.trigger", -1); // reset to 1000
Attachment #794918 -
Flags: review?(jdemooij)
Comment 4•11 years ago
|
||
Comment on attachment 794918 [details] [diff] [review]
Add generic shell function to toggle global compiler options.
Review of attachment 794918 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsapi.h
@@ +5040,1 @@
> typedef enum JSCompilerOption {
Can we rename this to JSJitCompilerOption while you're here? The API also uses "compiler" for bytecode compilation, see for instance the CompileOptions class.
::: js/src/shell/js.cpp
@@ +3291,5 @@
>
> #endif // JS_THREADSAFE
>
> +static bool
> +SetCompilerOption(JSContext *cx, unsigned argc, jsval *vp)
If we move this to builtin/TestingFunctions.cpp we can also use this function in the browser and (Jesse's) browser fuzzers can use it.
We should also add some jit-tests, so that the fuzzers know about this function.
@@ +3296,5 @@
> +{
> + CallArgs args = CallArgsFromVp(argc, vp);
> +
> + if (args.length() != 2)
> + return false;
Call ReportUsageError before returning false ("return false" without a pending exception will be treated as OOM)
@@ +3299,5 @@
> + if (args.length() != 2)
> + return false;
> +
> + if (!args[0].isString() || !args[1].isNumber())
> + return false;
Same here. Also, s/isNumber/isInt32, or the args[1].toInt32() below will assert if it's a double.
Attachment #794918 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•11 years ago
|
||
Delta:
- Apply nits.
- Rename JS_SetGlobalCompilerOption to JS_SetGlobalJitCompilerOption.
Attachment #794918 -
Attachment is obsolete: true
Attachment #795619 -
Flags: review?(jdemooij)
Comment 6•11 years ago
|
||
Comment on attachment 795619 [details] [diff] [review]
Add generic shell function to toggle global jit compiler options.
Review of attachment 795619 [details] [diff] [review]:
-----------------------------------------------------------------
Nice!
Attachment #795619 -
Flags: review?(jdemooij) → review+
Comment 7•11 years ago
|
||
I think this should be called setCompilerOption (without "Jit") because it's likely to be expanded to non-JIT things in the future. For example, it might grow a flag for turning on extra parser checks, or it might subsume gcparam.
Updated•11 years ago
|
Severity: normal → enhancement
Assignee | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf17ea43b59
With this patch the use case is like describe in comment 3, except that the function is named "setJitCompilerOption". The 2 options should remain the same, as documented in comment 3.
(In reply to Jesse Ruderman from comment #7)
> I think this should be called setCompilerOption (without "Jit") because it's
> likely to be expanded to non-JIT things in the future. For example, it
> might grow a flag for turning on extra parser checks, or it might subsume
> gcparam.
I jandem agree, I can make a follow-up patch to rename these.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 9•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=04a1713ab6f4
^ try run made before rebasing.
Comment 10•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 11•11 years ago
|
||
(In reply to Jesse Ruderman from comment #7)
> I think this should be called setCompilerOption (without "Jit") because it's
> likely to be expanded to non-JIT things in the future. For example, it
> might grow a flag for turning on extra parser checks, or it might subsume
> gcparam.
That's not unreasonable, but does "setCompilerOption" make a lot of sense for setting GC parameters etc? What does "Compiler" refer to in that case? Maybe setJSOption but that sounds really generic..
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #11)
> (In reply to Jesse Ruderman from comment #7)
> > I think this should be called setCompilerOption (without "Jit") because it's
> > likely to be expanded to non-JIT things in the future. For example, it
> > might grow a flag for turning on extra parser checks, or it might subsume
> > gcparam.
>
> That's not unreasonable, but does "setCompilerOption" make a lot of sense
> for setting GC parameters etc? What does "Compiler" refer to in that case?
> Maybe setJSOption but that sounds really generic..
The goal was to make it generic in the first place, this is also why I included a JSContext * as argument of this function, in case we might want to change or check against non-global options.
You need to log in
before you can comment on or make changes to this bug.
Description
•