Bug 908903 adds a testing function for changing the baseline and ion "trigger counts". I'd like it to also allow changing whether the JITs are enabled at all, whether TI is enabled, etc. This would allow me to greatly expand differential testing (see also bug 880330).

With bug 880330, how will I control which compartment I'm setting the options for? I will want to set options for a content compartment, but I have to call this privileged function from chrome.

This idea would be to take more options (such as the one tested by in IsIonEnabled[1]) and add this interface to the SetJitCompilerOption testing function added as part of Bug 908903. A second part to this issue would be to accept an optional argument, such as fuzzers can set an option on another window from the chrome code. So this second patch would involve modifying SetJitCompilerOption and potentially JS_SetGlobalJitCompilerOption to work with the options of the other compartment. Note: The JSContext* argument of SetJitCompilerOption is not yet used nor documented, so we can replace it by something more useful if needed. [1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Ion.h#l365

I'd like to take it up.

(In reply to iseki.m.aa from comment #3) > I'd like to take it up. I am sorry, you are currently assigned on Bug 900285, and I think it would be a good idea to finish the work that you started before taking another bug.

> I am sorry, you are currently assigned on Bug 900285, and I think it would > be a good idea to finish the work that you started before taking another bug. Is anyone else wanting to take on this bug? If not, then please take it, iseki.m.aa! Working on more than one bug at a time is common, and sensible given that compile times and test run times can be long.

I add interface to the SetJitCompilerOption. Is it Ok?

I add interface to the SetJitCompilerOption. Is it Ok?

I make "for" loop to accept more argument. I presume even number arguments is string and odd number arguments is int.

Comment on attachment 814234 [details] [diff] [review] bug909997-WIP.patch Review of attachment 814234 [details] [diff] [review]: ----------------------------------------------------------------- This patch is interesting but it does not answer the problematic of this bug. See comment 2, and ignore the second part for now, only focus on adding the enable flags as part of JS_SetGlobalJitCompilerOption[1]. [1] http://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp?from=JS_SetGlobalJitCompilerOption#l6102

(In reply to Nicolas B. Pierron [:nbp] from comment #10) Sorry, I can't understand well. > See comment 2, and ignore the second part for now, only focus on adding the > enable flags as part of JS_SetGlobalJitCompilerOption[1]. Enable flags is JSJITCOMPILER_BASELINE_USECOUNT_TRIGGER and JSJITCOMPILER_ION_USECOUNT_TRIGGER ? Should I make new flags such as JSOPTION_ION or JSOPTION_BASELINE ?

(In reply to iseki.m.aa from comment #11) > (In reply to Nicolas B. Pierron [:nbp] from comment #10) > > Sorry, I can't understand well. > > > See comment 2, and ignore the second part for now, only focus on adding the > > enable flags as part of JS_SetGlobalJitCompilerOption[1]. > Enable flags is JSJITCOMPILER_BASELINE_USECOUNT_TRIGGER and > JSJITCOMPILER_ION_USECOUNT_TRIGGER ? No, by "enable", I mean on/off flags. These are just identifier used to name a flag. > Should I make new flags such as JSOPTION_ION or JSOPTION_BASELINE ? Yes, JSOPTION_ION and JSOPTION_BASELINE are 2 flags. They are on/off (enable) flags. We need to change the value of these flags in JS_SetGlobalJitCompilerOption.

I make new flags JSOPTION_BASELINE_USECOUNT_TRIGGER and JSOPTION_ION_USECOUNT_TRIGGER. It is toggled in JS_SetGlobalJitCompilerOption.

Comment on attachment 814635 [details] [diff] [review] bug909997-WIP.patch Review of attachment 814635 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand the meaning of this patch. The idea is to switch JSOPTION_ION flag from JavaScript. Such as somebody can write in a JavaScript test case: setJitCompilerOption("ion.enable", 1); to enable IonMonkey. Have a look at the use cases of setJitCompilerOption and look into a debugger to follow the logic and understand where you have to add "ion.enable" string to make this work.

I understand what to do. thanks. I add JSJITCOMPILER_ION_ENABLE in switch and register it.

Comment on attachment 814685 [details] [diff] [review] bug909997-WIP.patch Review of attachment 814685 [details] [diff] [review]: ----------------------------------------------------------------- Good, this is the way to go. ::: js/src/jsapi.cpp @@ +6150,5 @@ > jit::js_IonOptions.setEagerCompilation(); > break; > + case JSJITCOMPILER_ION_ENABLE: > + if (value == 1) > + JS_ToggleOptions(cx, JSOPTION_ION); We do not want to toggle, we want to enable it, when the value is 1, and disable it when the value is 0. ::: js/src/jsapi.h @@ +4252,5 @@ > > #define JIT_COMPILER_OPTIONS(Register) \ > Register(BASELINE_USECOUNT_TRIGGER, "baseline.usecount.trigger") \ > + Register(ION_USECOUNT_TRIGGER, "ion.usecount.trigger") \ > + Register(ION_ENABLE, "ion.enable") And add the same thing for baseline.

I make JS_ResetOptions and add baseline flags.

Comment on attachment 814689 [details] [diff] [review] bug909997-wip.patch Review of attachment 814689 [details] [diff] [review]: ----------------------------------------------------------------- is this patch compiling?

Sorry, I didn't compile it and it includes syntax error. I fix it.

Comment on attachment 814703 [details] [diff] [review] bug909997.patch Review of attachment 814703 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this behave as expect, can you check in gdb that you optain the correct result at the end of JS_ResetOption? ::: js/src/jsapi.cpp @@ +977,5 @@ > +JS_ResetOptions(JSContext *cx, uint32_t options) > +{ > + unsigned oldopts = cx->options(); > + unsigned newopts = oldopts & !options; > + return SetOptionsCommon(cx, newopts); newopts is a bit field, right? Is the "!" operator a bit operator?

(In reply to Nicolas B. Pierron [:nbp] from comment #20) > I don't think this behave as expect, can you check in gdb that you optain > the correct result at the end of JS_ResetOption? JS_ResetOption same value below JS_SetOptions(cx, JSOPTION_ION); JS_ToggleOptions(cx, JSOPTION_ION); So, I think it works correct. > newopts is a bit field, right? > Is the "!" operator a bit operator? Yes, I think newopts is a bit field and "!" operator is a bit operator.

Sorry, I have miss type. × JS_ResetOption same value below ◯ JS_ResetOption return the same value below

I had been mistake that "not operatot" is "!" but it is "~".

Comment on attachment 815660 [details] [diff] [review] bug909997.patch Review of attachment 815660 [details] [diff] [review]: ----------------------------------------------------------------- Give a summary to your patch. Add test cases such as the mutation fuzzers can make use of it. Also check that the functions are not compiling if they are not supposed to compile. Have a look a the Hacking Tips[1] page of SpiderMonkey to see how to use Ion's spew. [1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Hacking_Tips ::: js/src/jsapi.cpp @@ +976,5 @@ > JS_PUBLIC_API(uint32_t) > +JS_ResetOptions(JSContext *cx, uint32_t options) > +{ > + unsigned oldopts = cx->options(); > + unsigned newopts = oldopts & (~options); nit: remove unnecessary parentheses. @@ +6160,5 @@ > + case JSJITCOMPILER_ION_ENABLE: > + if (value == 1) > + JS_SetOptions(cx, JSOPTION_ION); > + else if (value == 0) > + JS_ResetOptions(cx, JSOPTION_BASELINE); JSOPTION_BASELINE?

(In reply to Nicolas B. Pierron [:nbp] from comment #24) > Give a summary to your patch. > > Add test cases such as the mutation fuzzers can make use of it. Also check > that the functions are not compiling if they are not supposed to compile. > Have a look a the Hacking Tips[1] page of SpiderMonkey to see how to use > Ion's spew. > > [1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Hacking_Tips Sorry, I can't understand well. Its mean is to compile setJitCompilerOption("logs.enable", 1); setJitCompilerOption("scripts.enable", 1); so on ? the meaning of fuzzers is operation including error ?

(In reply to iseki.m.aa from comment #25) > (In reply to Nicolas B. Pierron [:nbp] from comment #24) > > Give a summary to your patch. > > > > Add test cases such as the mutation fuzzers can make use of it. Also check > > that the functions are not compiling if they are not supposed to compile. > > Have a look a the Hacking Tips[1] page of SpiderMonkey to see how to use > > Ion's spew. > > > > [1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Hacking_Tips > > Sorry, I can't understand well. Is that a problem of English, am I not clear enough? Can I do anything to help you understand well in the future? > Its mean is to compile > setJitCompilerOption("logs.enable", 1); > setJitCompilerOption("scripts.enable", 1); > so on ? No, I meant: use setJitCompilerOption("ion.enable", 1); in a test case such as when you use "IONFLAGS=bl-scripts,scripts ./js test-case.js", you can see a difference if you replace the 1 by a 0. And same thing with "baseline.enable". > the meaning of fuzzers is operation including error ? A fuzzer is a tool which is used for fuzzing[1]. [1] http://en.wikipedia.org/wiki/Fuzz_testing

(In reply to Nicolas B. Pierron [:nbp] from comment #26) > (In reply to iseki.m.aa from comment #25) > > (In reply to Nicolas B. Pierron [:nbp] from comment #24) > > > Give a summary to your patch. > > > > > > Add test cases such as the mutation fuzzers can make use of it. Also check > > > that the functions are not compiling if they are not supposed to compile. > > > Have a look a the Hacking Tips[1] page of SpiderMonkey to see how to use > > > Ion's spew. > > > > > > [1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Hacking_Tips > > > > Sorry, I can't understand well. > > Is that a problem of English, am I not clear enough? Can I do anything to > help you understand well in the future? Thanks:) My English skill is so poor. So I ask you many times. > > the meaning of fuzzers is operation including error ? > > A fuzzer is a tool which is used for fuzzing[1]. > > [1] http://en.wikipedia.org/wiki/Fuzz_testing Thanks:)

Comment on attachment 816310 [details] [diff] [review] bug909997WIP.patch Review of attachment 816310 [details] [diff] [review]: ----------------------------------------------------------------- Create a test named bug909997.js in js/src/jit-test/tests/ion/ This test should cover all new use cases of setJitCompilerOption added by this patch. This means the 4 following statements should appear in the test case: setJitCompilerOption("ion.enable", 0); setJitCompilerOption("ion.enable", 1); setJitCompilerOption("baseline.enable", 0); setJitCompilerOption("baseline.enable", 1); Then add the test case to your patch. Run the "make check" command to verify that the tests are working. In the test case, make sure to add a function which is supposed to run in both baseline and IonMonkey if none of the configurations are modified. ::: js/src/jsapi.cpp @@ +6159,5 @@ > break; > + case JSJITCOMPILER_ION_ENABLE: > + if (value == 1) { > + JS_SetOptions(cx, JSOPTION_ION); > + IonSpew(js::jit::IonSpew_BaselineScripts,"Enable ion"); nit: Add one space after the comma. @@ +6163,5 @@ > + IonSpew(js::jit::IonSpew_BaselineScripts,"Enable ion"); > + } > + else if (value == 0) { > + JS_ResetOptions(cx, JSOPTION_ION); > + IonSpew(js::jit::IonSpew_BaselineScripts,"Disable ion"); nit: same here. @@ +6169,5 @@ > + break; > + case JSJITCOMPILER_BASELINE_ENABLE: > + if (value == 1) { > + JS_SetOptions(cx, JSOPTION_BASELINE); > + IonSpew(js::jit::IonSpew_BaselineScripts,"Enable baseline"); nit: same here. @@ +6173,5 @@ > + IonSpew(js::jit::IonSpew_BaselineScripts,"Enable baseline"); > + } > + else if (value == 0) { > + JS_ResetOptions(cx, JSOPTION_BASELINE); > + IonSpew(js::jit::IonSpew_BaselineScripts,"Disable baseline"); nit: same here.

I run "make check". bug909997.js was included. TEST-PASS | js/src/jit-test/tests/ion/bug909997.js | TEST-PASS | js/src/jit-test/tests/ion/bug909997.js | --ion-eager TEST-PASS | js/src/jit-test/tests/ion/bug909997.js | --ion-eager --ion-check-range-analysis TEST-PASS | js/src/jit-test/tests/ion/bug909997.js | --baseline-eager TEST-PASS | js/src/jit-test/tests/ion/bug909997.js | --baseline-eager --no-ti --no-fpu TEST-PASS | js/src/jit-test/tests/ion/bug909997.js | --no-baseline --no-ion

Comment on attachment 816404 [details] [diff] [review] bug909997.patch Review of attachment 816404 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/ion/bug909997.js @@ +1,1 @@ > +setJitCompilerOption("ion.enable", 0); Add a function and call it enough times such as IonMonkey would be supposed to trigger a compilation is it was not disabled. And do the same after each call to setJitCompilerOption.

I add "for loop". This loop run 1000 times. After adding this loop, Baseline scripts return below [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:3 (0x10253f1c0) [BaselineScripts] Created BaselineScript 0x1021220a0 (raw 0x1017e5360) for ./jit-test/tests/ion/bug909997.js:3 But, ion scripts return nothing.

(In reply to iseki.m.aa from comment #33) > Created attachment 816431 [details] [diff] [review] > bug909997.patch > > I add "for loop". > This loop run 1000 times. > After adding this loop, Baseline scripts return below > [BaselineScripts] Baseline compiling script > ./jit-test/tests/ion/bug909997.js:3 (0x10253f1c0) > [BaselineScripts] Created BaselineScript 0x1021220a0 (raw 0x1017e5360) for > ./jit-test/tests/ion/bug909997.js:3 > But, ion scripts return nothing. Both IonMonkey and the Baseline Compiler are method compilers. This mean that they cannot compile less than one function. I think you should create 4 functions and each has a hot-loop inside. Then test the 4 configurations by settings the jit options before calling each function.

Comment on attachment 816877 [details] [diff] [review] bug909997.patch Review of attachment 816877 [details] [diff] [review]: ----------------------------------------------------------------- What is the result given by IONFLAGS=baseline-scripts,scripts ? ::: js/src/jit-test/tests/ion/bug909997.js @@ +18,5 @@ > + > +var func = [method_A, method_B, method_C, method_D] > + > +for (var n = 0; n < 4; ++n) { > + setJitCompilerOption("ion.enable", n&1); Add spaces around & operators. @@ +19,5 @@ > +var func = [method_A, method_B, method_C, method_D] > + > +for (var n = 0; n < 4; ++n) { > + setJitCompilerOption("ion.enable", n&1); > + setJitCompilerOption("baseline.enable", Math.floor(n/2)); Math.floor(n/2) → ((n & 2) ? 1 : 0) @@ +20,5 @@ > + > +for (var n = 0; n < 4; ++n) { > + setJitCompilerOption("ion.enable", n&1); > + setJitCompilerOption("baseline.enable", Math.floor(n/2)); > + for (var i = 0; i < func.length; ++i) Remove this for loop @@ +21,5 @@ > +for (var n = 0; n < 4; ++n) { > + setJitCompilerOption("ion.enable", n&1); > + setJitCompilerOption("baseline.enable", Math.floor(n/2)); > + for (var i = 0; i < func.length; ++i) > + func[i](); Replace i by n.

Comment on attachment 817527 [details] [diff] [review] bug909997.patch Review of attachment 817527 [details] [diff] [review]: ----------------------------------------------------------------- Please reply to the question, there is no need to attach a new patch all the time.

(In reply to Nicolas B. Pierron [:nbp] from comment #38) > Comment on attachment 817527 [details] [diff] [review] > bug909997.patch > > Review of attachment 817527 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please reply to the question, there is no need to attach a new patch all the > time. I'm sorry. The result given by IONFLAGS=baseline-scripts,scripts is below [Scripts] Disable ion [BaselineScripts] Disable baseline [Scripts] Enable ion [BaselineScripts] Disable baseline [Scripts] Disable ion [Scripts] Enable ion

(In reply to iseki.m.aa from comment #39) > (In reply to Nicolas B. Pierron [:nbp] from comment #38) > > Comment on attachment 817527 [details] [diff] [review] > > bug909997.patch > > > > Review of attachment 817527 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Please reply to the question, there is no need to attach a new patch all the > > time. > I'm sorry. > > The result given by IONFLAGS=baseline-scripts,scripts is below > [Scripts] Disable ion > [BaselineScripts] Disable baseline > [Scripts] Enable ion > [BaselineScripts] Disable baseline > [Scripts] Disable ion > [Scripts] Enable ion This means that things are not working as expect, right?

(In reply to Nicolas B. Pierron [:nbp] from comment #40) > (In reply to iseki.m.aa from comment #39) > > (In reply to Nicolas B. Pierron [:nbp] from comment #38) > > > Comment on attachment 817527 [details] [diff] [review] > > > bug909997.patch > > > > > > Review of attachment 817527 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > Please reply to the question, there is no need to attach a new patch all the > > > time. > > I'm sorry. > > > > The result given by IONFLAGS=baseline-scripts,scripts is below > > [Scripts] Disable ion > > [BaselineScripts] Disable baseline > > [Scripts] Enable ion > > [BaselineScripts] Disable baseline > > [Scripts] Disable ion > > [Scripts] Enable ion > > This means that things are not working as expect, right? Sorry. I was mistyped. I change "n & 2" to "n & 2 ? 1 : 0". The result given by IONFLAGS=baseline-scripts,scripts is below [Scripts] Disable ion [BaselineScripts] Disable baseline [Scripts] Enable ion [BaselineScripts] Disable baseline [Scripts] Disable ion [BaselineScripts] Enable baseline [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:11 (0x10253f340) [BaselineScripts] Created BaselineScript 0x1021114b0 (raw 0x1017e50b8) for ./jit-test/tests/ion/bug909997.js:11 [Scripts] Enable ion [BaselineScripts] Enable baseline [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:15 (0x10253f400) [BaselineScripts] Created BaselineScript 0x1021118d0 (raw 0x1017e53b0) for ./jit-test/tests/ion/bug909997.js:15 Baseline Compiler compile method. but IonMonkey didn't compile method.

(In reply to iseki.m.aa from comment #41) > [Scripts] Enable ion > [BaselineScripts] Enable baseline > [BaselineScripts] Baseline compiling script > ./jit-test/tests/ion/bug909997.js:15 (0x10253f400) > [BaselineScripts] Created BaselineScript 0x1021118d0 (raw 0x1017e53b0) for > ./jit-test/tests/ion/bug909997.js:15 > > Baseline Compiler compile method. but IonMonkey didn't compile method. Why is IonMonkey not compiling?

(In reply to Nicolas B. Pierron [:nbp] from comment #42) > Why is IonMonkey not compiling? I thought IonMonkey display the message like Baseline compiler such as "[BaselineScripts] Baseline compiling script". But I can't find similar message in the source code. Is IonMonkey compile method correctly ?

(In reply to iseki.m.aa from comment #43) > (In reply to Nicolas B. Pierron [:nbp] from comment #42) > > Why is IonMonkey not compiling? > I thought IonMonkey display the message like Baseline compiler such as > "[BaselineScripts] Baseline compiling script". > But I can't find similar message in the source code. > Is IonMonkey compile method correctly ? If you have any doubt, test it your-self. Maybe try with --ion-eager.

I comment out the "setJitCompilerOption("ion.enable", n & 1)" and "setJitCompilerOption("baseline.enable", n & 2 ? 1 : 0)". The result given by IONFLAGS=baseline-scripts,scripts is below > [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:3 (0x10253f1c0) > [BaselineScripts] Created BaselineScript 0x102121460 (raw 0x1017e5150) for ./jit-test/tests/ion/bug909997.js:3 > [Scripts] Analyzing script ./jit-test/tests/ion/bug909997.js:3 (0x10253f1c0) (usecount=1100) > [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:7 (0x10253f280) > [BaselineScripts] Created BaselineScript 0x102121f30 (raw 0x1017e5648) for ./jit-test/tests/ion/bug909997.js:7 > [Scripts] Analyzing script ./jit-test/tests/ion/bug909997.js:7 (0x10253f280) (usecount=1100) > [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:11 (0x10253f340) > [BaselineScripts] Created BaselineScript 0x102122330 (raw 0x1017e5888) for ./jit-test/tests/ion/bug909997.js:11 > [Scripts] Analyzing script ./jit-test/tests/ion/bug909997.js:11 (0x10253f340) (usecount=1100) > [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:15 (0x10253f400) > [BaselineScripts] Created BaselineScript 0x1021229d0 (raw 0x1017e5ac8) for ./jit-test/tests/ion/bug909997.js:15 > [Scripts] Analyzing script ./jit-test/tests/ion/bug909997.js:15 (0x10253f400) (usecount=1100) So, previous result didn't work correctly. If this test work correctly , the result would be below > [Scripts] Disable ion > [BaselineScripts] Disable baseline > [Scripts] Enable ion > [BaselineScripts] Disable baseline > [Scripts] Analyzing script ./jit-test/tests/ion/bug909997.js:7 (0x10253f280) (usecount=1100) > [Scripts] Disable ion > [BaselineScripts] Enable baseline > [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:11 (0x10253f340) > [BaselineScripts] Created BaselineScript 0x10160d710 (raw 0x1017e50b8) for ./jit-test/tests/ion/bug909997.js:11 > [Scripts] Enable ion > [BaselineScripts] Enable baseline > [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:15 (0x10253f400) > [BaselineScripts] Created BaselineScript 0x10160db30 (raw 0x1017e53b0) for ./jit-test/tests/ion/bug909997.js:15 > [Scripts] Analyzing script ./jit-test/tests/ion/bug909997.js:15 (0x10253f400) (usecount=1100) But I don't know why "setJitCompilerOption("ion.enable", n & 1)" didn't work correctly.

I have two mistake. 1. I had thought setJitCompilerOption can be used to overlap. 2. I had thought to use IonMonkey , It's enought to set JSOPTION_ION. But IsIonEnabled also check JSOPTION_BASELINE. I modify the patch. The result given by IONFLAGS=baseline-scripts,scripts is below. > [BaselineScripts] Disable baseline > [BaselineScripts] Enable baseline > [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:7 (0x10263f280) > [BaselineScripts] Created BaselineScript 0x10300a990 (raw 0x1017e50b8) for ./jit- test/tests/ion/bug909997.js:7 > [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:1 (0x10263f100) > [BaselineScripts] Created BaselineScript 0x103821200 (raw 0x1017e5718) for ./jit-test/tests/ion/bug909997.js:1 > [Scripts] Disable ion > [Scripts] Enable ion > [BaselineScripts] Baseline compiling script ./jit-test/tests/ion/bug909997.js:15 (0x10263f400) > [BaselineScripts] Created BaselineScript 0x102411220 (raw 0x1017e6a28) for ./jit-test/tests/ion/bug909997.js:15 > [Scripts] Analyzing script ./jit-test/tests/ion/bug909997.js:15 (0x10263f400) (usecount=1022) This result is expected.

Comment on attachment 817670 [details] [diff] [review] bug909997.patch Review of attachment 817670 [details] [diff] [review]: ----------------------------------------------------------------- Nice work.

Comment on attachment 817670 [details] [diff] [review] bug909997.patch Review of attachment 817670 [details] [diff] [review]: ----------------------------------------------------------------- cancel the checkin flag, there is still some style issues to be fixed. ::: js/src/jsapi.cpp @@ +6161,5 @@ > + if (value == 1) { > + JS_SetOptions(cx, JSOPTION_BASELINE | JSOPTION_ION); > + IonSpew(js::jit::IonSpew_Scripts, "Enable ion"); > + } > + else if (value == 0) { style-nit: move the else on the previous line. @@ +6171,5 @@ > + if (value == 1) { > + JS_SetOptions(cx, JSOPTION_BASELINE); > + IonSpew(js::jit::IonSpew_BaselineScripts, "Enable baseline"); > + } > + else if (value == 0) { style-nit: move the else on the previous line.

Backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/a88f40be25e7 https://tbpl.mozilla.org/php/getParsedLog.php?id=29263774&tree=Mozilla-Inbound

Masaya, I am not sure how you managed to test the patch knowing that there is a fatal compilation error. Can you investigate what is wrong with your build. Then fix the patch to remove this compilation error.

Sorry, I didn't update. cx->options() command change GetOptionsCommon(cx).

Comment on attachment 818790 [details] [diff] [review] bug909997.patch Review of attachment 818790 [details] [diff] [review]: ----------------------------------------------------------------- Ok, I did not noticed before, but JS_SetOption will reset all options with the new set of options that you are giving, which means that when you enable baseline, you also disable Ion (even if baseline was already activated). ::: js/src/jsapi.cpp @@ +1023,5 @@ > return SetOptionsCommon(cx, options); > } > > JS_PUBLIC_API(uint32_t) > +JS_ResetOptions(JSContext *cx, uint32_t options) rename JS_DisableOptions and add JS_EnableOptions @@ +6200,5 @@ > jit::js_IonOptions.setEagerCompilation(); > break; > + case JSJITCOMPILER_ION_ENABLE: > + if (value == 1) { > + JS_SetOptions(cx, JSOPTION_BASELINE | JSOPTION_ION); use JS_EnableOption instead of SetOptions, as we do not want to reset all other flags, and remove JSOPTION_BASELINE from ION_ENABLE.

1) rename JS_ResetOptions to JS_DisableOptions. 2) add JS_EnableOptions 3) use JS_EnableOption instead of SetOptions. 4) remove JSOPTION_BASELINE from ION_DISABLE 5) modify test case. > use JS_EnableOption instead of SetOptions, as we do not want to reset all other flags, and remove JSOPTION_BASELINE from ION_ENABLE. IsIonEnabled check JSOPTION_BASELINE and JSOPTION_ION. So I can't remove JSOPTION_BASELINE from ION_ENABLE. Previous test case have finished while enable flags. This test through below. JS_EnableOptions(cx, JSOPTION_ION); So I modified test case to finish while disable flags.

Comment on attachment 820151 [details] [diff] [review] bug909997.patch Review of attachment 820151 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +6208,5 @@ > jit::js_IonOptions.setEagerCompilation(); > break; > + case JSJITCOMPILER_ION_ENABLE: > + if (value == 1) { > + JS_EnableOptions(cx, JSOPTION_BASELINE | JSOPTION_ION); I think this would be better if it would be symmetric, even if this does not enable Ion because of some other flags which also have to be switched on.

1) remove JSOPTION_BASELINE from ION_DISABLE. 2) modify test case.

Comment on attachment 820734 [details] [diff] [review] bug909997.patch Review of attachment 820734 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good. :) Thanks for documenting the changes between patches.

Pushed this along with a bunch of other checkin-neededs to try: https://tbpl.mozilla.org/?tree=Try&rev=503271ae46d1

(The commit message had quotes around the string, have removed manually, but please double check for this before attaching another time :-))

(In reply to Ed Morley [:edmorley UTC+1] from comment #60) > (The commit message had quotes around the string, have removed manually, but > please double check for this before attaching another time :-)) Sorry. Thanks:)

(In reply to Nicolas B. Pierron [:nbp] from comment #2) > A second part to this issue would be to accept an optional argument, such as > fuzzers can set an option on another window from the chrome code. So this > second patch would involve modifying SetJitCompilerOption and potentially > JS_SetGlobalJitCompilerOption to work with the options of the other > compartment. The second part is unfortunately not doable unless options are moved outside the JSContext to the Compartment, as it should be as soon as Bug 880330 is complete.

We can advance second part because options are moved outside the JSContext to the Compartment. A second part is to accept an optiocal argument. Is my previous patch(attachment 814234 [details] [diff] [review] in Comment 9) correct?

(In reply to iseki.m.aa from comment #65) > We can advance second part because options are moved outside the JSContext > to the Compartment. > > A second part is to accept an optiocal argument. > Is my previous patch(attachment 814234 [details] [diff] [review] in Comment > 9) correct? No, the goal of the second part is to find the compartment of a third/this argument and set the flag on its compartment. var g = newGlobal(); g.setJitCompilerOption("ion.enable", 1); or var g = newGlobal(); setJitCompilerOption("ion.enable", 1, g);

I make getJitCompilerOption to confirm options easily.

I have misunderstood. JIT options still don't move out of JSContext.

This bug fixed in bug940676.

Can you *please* wait to resolve the bug until after said patch lands? It's really confusing for bug archaeology to resolve a bug when the bug it depends on hasn't landed yet.

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #70) > Can you *please* wait to resolve the bug until after said patch lands? It's > really confusing for bug archaeology to resolve a bug when the bug it > depends on hasn't landed yet. I'm sorry. I'm careful in the future.

Sorry, I misunderstand. This bug had not still fixed.

Does anyone know the status of this bug? This feature seems useful.

This work is complete and landed. We have the {Set,Get}JitCompilerOption functions which are part of the testing functions, and we used these for making test cases shorter, or in some special cases to ensure that we always run in the same context.