Closed Bug 945596 Opened 6 years ago Closed 6 years ago

JIT compiler options tests is incomplete.

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: isk, Assigned: isk)

Details

Attachments

(1 file, 5 obsolete files)

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.
Attached patch bug945596.patch (obsolete) — Splinter Review
Sorry, I forgot to add patch.
Attachment #8341550 - Flags: review?(nicolas.b.pierron)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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
Attachment #8341550 - Flags: review?(nicolas.b.pierron)
Attached patch bug945596.patch (obsolete) — Splinter Review
Thanks for your reviewing.

I fix the code pointed out and test case.
Attachment #8341550 - Attachment is obsolete: true
Attachment #8342869 - Flags: review?(nicolas.b.pierron)
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);
Attachment #8342869 - Flags: review?(nicolas.b.pierron)
Attached patch bug945596.patch (obsolete) — Splinter Review
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.
Attachment #8342869 - Attachment is obsolete: true
Attachment #8344448 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8344448 - Flags: review?(nicolas.b.pierron)
Attached patch bug945596.patch (obsolete) — Splinter Review
Thank you for your reviewing.

I fixed the code which pointed out.
Macros is very useful to reduce the code.
Attachment #8344448 - Attachment is obsolete: true
Attachment #8345720 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8345720 [details] [diff] [review]
bug945596.patch

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

Awesome!
Attachment #8345720 - Flags: review?(nicolas.b.pierron) → review+
(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
Keywords: checkin-needed
(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.)
Attached patch Bug945596.patch (obsolete) — Splinter Review
Thank you for your information.

I fix this patch.

In such situation, This patch needs review+?
If so, could you review this patch?
Attachment #8345720 - Attachment is obsolete: true
Attachment #8355423 - Flags: review?(hv1989)
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.
Attachment #8355423 - Flags: review?(hv1989) → review+
Thank you for your reviewing.

I push try-server.
https://tbpl.mozilla.org/?tree=Try&rev=1abdfec05dff
Whiteboard: [checkin-needed]
Could you please update the patch as requested? Thanks
Whiteboard: [checkin-needed]
Attached patch Bug945596.patchSplinter Review
(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
Attachment #8355423 - Attachment is obsolete: true
Comment on attachment 8356069 [details] [diff] [review]
Bug945596.patch

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

Thanks :D
Attachment #8356069 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a9d9eb8a7d88
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.