945596 - JIT compiler options tests is incomplete.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Masaya Iseki[:isk](UTC+9)]

I make JS compiler options test in bug909997.
This tests is incomplete because this test don't assert value.

I add getJitCompilerOption to confirm JIT compiler option and modify previous test to assert JIT compiler option.

Comment 1

[Masaya Iseki[:isk](UTC+9)]

Attachment: bug945596.patch

Sorry, I forgot to add patch.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8341550 [details] [diff] [review]
bug945596.patch

Review of attachment 8341550 [details] [diff] [review]:
-----------------------------------------------------------------

This is really awesome idea to create an object to make these observable, but I think this would be better if it was as a parallel of SetJitCompilerOption, otherwise the name is confusing.
I think you should at least make the JitCompilerOptions visible in the value returned by this function:

{ ion : { enable : 1, usecount: { trigger : 1000 } }
, baseline : { enable : 1, usecount: { trigger : 10 } }
}

or

{ "ion.enable" : 1
, "ion.usecount.trigger" : 1000
, "baseline.enable" : 1
, "baseline.usecount.trigger" : 10
}

Adding the ContextOptions is nice, but their names is refused as an input of SetJitCompilerOptions

Comment 3

[Masaya Iseki[:isk](UTC+9)]

Attachment: bug945596.patch

Thanks for your reviewing.

I fix the code pointed out and test case.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8342869 [details] [diff] [review]
bug945596.patch

Review of attachment 8342869 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TestingFunctions.cpp
@@ +1145,5 @@
> +            return false;
> +
> +        value.setObject(*usecount);
> +        if (!JS_SetProperty(cx, compiler, "usecount", value))
> +            return false;

Could we do the same thing with the macros, such as we can easily extend this functions just by adding one line to define the new option in the Setter and Getter?

@@ +1641,5 @@
>  "setCompilerOption(<option>, <number>)",
>  "  Set a compiler option indexed in JSCompileOption enum to a number.\n"),
>  
> +    JS_FN_HELP("getJitCompilerOption", GetJitCompilerOption, 0, 0,
> +"getCompilerOption()",

getJitCompilerOption*s*(), otherwise you would query one option at a time and the prototype would be getJitCompilerOption(<option>).

::: js/src/jit-test/tests/ion/bug909997.js
@@ +21,5 @@
>  for (var n = 0; n < 4; ++n) {
>      setJitCompilerOption("baseline.enable", n & 1);
>      setJitCompilerOption("ion.enable", n & 2 ? 1: 0);
> +    var opt = getJitCompilerOption();
> +    assertEq(opt.baseline.enable, n & 1 ? true :false);

if you use property names with a "." in the name, which is likely if you are doing to use the macro to generate these, then you might have to change the condition from

  assertEq(opt.baseline.enable, n & 1 ? true :false);

to

  assertEq(opt["baseline.enable"], n & 1 ? true :false);

Comment 5

[Masaya Iseki[:isk](UTC+9)]

Attachment: bug945596.patch

Thanks you for your reviewing.

I think assertEq(opt["baseline.enable"], n & 1 ? true :false) is more smart than assertEq(opt.baseline.enable, n & 1 ? true :false).
So I change it.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8344448 [details] [diff] [review]
bug945596.patch

Review of attachment 8344448 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, I think we can generalize a bit more by using the same kind of Macro as SetJitCompilerOption.

If you have any trouble to understand the macro you can ask me on IRC, or read http://journal.stuffwithstuff.com/2012/01/24/higher-order-macros-in-c/

::: js/src/builtin/TestingFunctions.cpp
@@ +1128,5 @@
> +    RootedValue value(cx);
> +
> +    value = JS::ContextOptionsRef(cx).baseline()
> +            ? BooleanValue(true)
> +            : BooleanValue(false);

Can you use value.setInt32(…) instead of BooleanValue(true) and BooleanValue(false) ?

@@ +1138,5 @@
> +        return false;
> +
> +    value = JS::ContextOptionsRef(cx).ion()
> +            ? BooleanValue(true)
> +            : BooleanValue(false);

And the same here.

@@ +1144,5 @@
> +        return false;
> +
> +    value.setInt32(jit::js_IonOptions.baselineUsesBeforeCompile);
> +    if (!JS_SetProperty(cx, info, "ion.usecount.trigger", value))
> +        return false;

Now that you have almost 4 times the same statements, can you add a function named JS_GetGlobalJitCompilerOption.  This function should return an int32.  This value would be given as argument of value.setInt32(…).

And now you can do the same thing as in http://dxr.mozilla.org/mozilla-central/source/js/src/builtin/TestingFunctions.cpp#1097 which would give us the ability to ignore this code in the future. :)

::: js/src/jit-test/tests/ion/bug909997.js
@@ +21,5 @@
>  for (var n = 0; n < 4; ++n) {
>      setJitCompilerOption("baseline.enable", n & 1);
>      setJitCompilerOption("ion.enable", n & 2 ? 1: 0);
> +    var opt = getJitCompilerOptions();
> +    assertEq(opt["baseline.enable"], n & 1 ? true :false);

You call the previous function with integer values, and you are comparing with boolean value.  This is inconsistent.

Comment 7

[Masaya Iseki[:isk](UTC+9)]

Attachment: bug945596.patch

Thank you for your reviewing.

I fixed the code which pointed out.
Macros is very useful to reduce the code.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8345720 [details] [diff] [review]
bug945596.patch

Review of attachment 8345720 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome!

Comment 9

[Masaya Iseki[:isk](UTC+9)]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Comment on attachment 8345720 [details] [diff] [review]
> bug945596.patch
> 
> Review of attachment 8345720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome!

Thank you for your reviewing.

I push to try-server.
https://tbpl.mozilla.org/?tree=Try&rev=d825cdaed1f4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4cd38abdc5

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/709faef751b8

https://tbpl.mozilla.org/php/getParsedLog.php?id=32473889&tree=Mozilla-Inbound

Comment 12

[Hannes Verschore [:h4writer]]

(In reply to iseki.m.aa from comment #9)
> Thank you for your reviewing.
> 
> I push to try-server.
> https://tbpl.mozilla.org/?tree=Try&rev=d825cdaed1f4

Some things have changed during 12th of December and now. Bug 939614 changes some things. So that's the reason this patch (which was correct before) needs some minor adjustments.

1) js_IonOptions.baselineUsesBeforeCompile is now jit::js_JitOptions.baselineUsesBeforeCompile
2) IonMonkey now has the possibility to have different optimization levels with different "UsesBeforeCompile". Currently we only have one "LEVEL" enabled (Optimization_Normal). The uses can be found through: jit::js_IonOptimizations.get(jit::Optimization_Normal)->usesBeforeCompile_

I think that are the only changes needed.
For (2) would it be possible to add "normal" in the name? So it is more clear when we start to have more compilation levels?

(It is a bit unfortunate this didn't land before 19th of December, else I would have adjusted the code when landing my changes. I hope this gives enough information into what needs to get changed in order to be commit ready again? Don't hesitate to ask me for more information.)

Comment 13

[Masaya Iseki[:isk](UTC+9)]

Attachment: Bug945596.patch

Thank you for your information.

I fix this patch.

In such situation, This patch needs review+?
If so, could you review this patch?

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 8355423 [details] [diff] [review]
Bug945596.patch

Review of attachment 8355423 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! You can add me and nbp as reviewers. It's not needed to ask another review for the small oversight of me. You can just fix it and upload again, r+ yourself (with comment: "taking r+ over from previous patch") and add the checkin needed question.

::: js/src/jsapi.cpp
@@ +6047,5 @@
> +    switch (opt) {
> +      case JSJITCOMPILER_BASELINE_USECOUNT_TRIGGER:
> +        return jit::js_JitOptions.baselineUsesBeforeCompile;
> +      case JSJITCOMPILER_ION_USECOUNT_TRIGGER:
> +        return jit::js_IonOptimizations.get(jit::Optimization_Normal)->usesBeforeCompile_;

Okay my bad. This should mimick what JS_SetGlobalJitCompilerOption does, right. In that case it should be:

return jit::js_JitOptions.forcedDefaultIonUsesBeforeCompile;

Sorry about forgetting that.

Comment 15

[Masaya Iseki[:isk](UTC+9)]

Thank you for your reviewing.

I push try-server.
https://tbpl.mozilla.org/?tree=Try&rev=1abdfec05dff

Comment 16

[Hannes Verschore [:h4writer]]

Could you please update the patch as requested? Thanks

Comment 17

[Masaya Iseki[:isk](UTC+9)]

Attachment: Bug945596.patch

(In reply to Hannes Verschore [:h4writer] from comment #16)
> Could you please update the patch as requested? Thanks

I'm sorry. I misunderstand.

Taking r+ over from previous patch.
I push try-server.

https://tbpl.mozilla.org/?tree=Try&rev=dcc9786c691e

Comment 18

[Hannes Verschore [:h4writer]]

Comment on attachment 8356069 [details] [diff] [review]
Bug945596.patch

Review of attachment 8356069 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks :D

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d9eb8a7d88

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a9d9eb8a7d88