Closed Bug 825599 Opened 7 years ago Closed 7 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: nbp, Unassigned)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

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.
(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.
No longer depends on: 813425
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")) { … }
Attachment #696835 - Flags: review?(hv1989)
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.
Attachment #696835 - Flags: review?(hv1989)
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.
Attachment #696835 - Attachment is obsolete: true
Attachment #697004 - Flags: review?(hv1989)
Depends on: 825892
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.
Attachment #697004 - Flags: review?(hv1989) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 839315
No longer depends on: 839315
You need to log in before you can comment on or make changes to this bug.