825599 - IonMonkey: Octane, pdfjs.js:16934: Use CompareV (StrictlyEqual) for String comparison.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

The StrictlyEqual function is likely coming from the conditional switch used in this function, as there is no strict equality manually written in this function.

This issue currently account for 3% for the PdfJS octane benchmark, likely more as soon as Bug 813425 is fixed as it caused extra bailouts returning in JM.

Comment 1

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #0)
> This issue currently account for 3% for the PdfJS octane benchmark, likely
> more as soon as Bug 813425 is fixed as it caused extra bailouts returning in
> JM.

Correction, most of the string comparisons done by this function are made before the bailout which happen before the call to decrypt functions which are used to get the glyph of the font.  So these invalidations would not cause any increase of the significance of this issue.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Specialize type switch's conditions.

This patch use TI to specialize JSOP_CASE of conditional switch and use the local type define by the expression computed for the switch condition likely typed by TI (*) and provide a String specialization for appropriate operations.

This patch improves PdfJS octane benchmark by 8% by saving the boxing of the string and the VM function call.  As opposed as I mentioned in previous comments, Bug 813425, on top of which this measurement is made, helps because it removes the invalidation which caused the beginning of this function to run mostly in JM instead of Ion.

(*) This won't work across Phi nodes such as   switch ((c ? "a" : "b")) { … }

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 696835 [details] [diff] [review]
Specialize type switch's conditions.

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

::: js/src/ion/IonBuilder.cpp
@@ +2316,5 @@
>      bool prevShared = false;
>      jsbytecode *prevpc = NULL;
>      for (unsigned int i = 0; i < ncases; i++) {
>          Value rval = script()->getConst(GET_UINT32_INDEX(pc2));
> +        JS_ASSERT(rval.isString());

This isn't true! lookupswitch can have other types as input and even different types:
  switch (i) {
    case 1:
    case "1000":
      return i;
  }

@@ +2334,5 @@
>  
>          MCompare *cmpIns = MCompare::New(ins, rvalIns, JSOP_STRICTEQ);
> +        // If it is computed in a previous block, it should have the correct type.
> +        if (ins->type() == MIRType_String)
> +            cmpIns->setCompareType(MCompare::Compare_String);

I assume we can't use infer here, because TI doesn't have any information about this compare? Can we take the TI information of types that flows into the switch (X) and infer the type of comparison with the type of the rval? I also would prefer to have a function (on MCompare) to do this, instead of in IonBuilder.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Specialize type switch's conditions.

Good catch, I was not aware that lookup switch could be used with anything else than strings.  As this did not break any existing test, I added a test case to check that we no compilation failure happen, and checked manually that the type inference generate the correct specialization.

I also added a subset of MCompare::infer which use only the MIRType to specialize the kind of comparisons.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 697004 [details] [diff] [review]
Specialize type switch's conditions.

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

::: js/src/ion/IonBuilder.cpp
@@ +2649,5 @@
>          MDefinition *caseOperand = current->pop();
>          MDefinition *switchOperand = current->peek(-1);
>          MCompare *cmpResult = MCompare::New(switchOperand, caseOperand, JSOP_STRICTEQ);
> +        TypeOracle::BinaryTypes b = oracle->binaryTypes(script(), pc);
> +        cmpResult->infer(b, cx);

Per bug 825892 all changes to lookupswitch aren't needed anymore. So I'm giving r+ for this change, IonSpewer.cpp and testcase.

Comment 6

[Nicolas B. Pierron [:nbp]]

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

Comment 7

[Phil Ringnalda (:philor)]

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