Closed Bug 1483189 Opened 4 years ago Closed 4 years ago

Performance regression in Ares-6/Basic due to porting Compare IC to CacheIR


(Core :: JavaScript Engine: JIT, defect)

Not set



Tracking Status
firefox63 --- fixed


(Reporter: anba, Assigned: mgaudet)


(Blocks 1 open bug)



(2 files, 1 obsolete file)

bug 1341261, part 7 led to a performance regression in Ares-6/Basic (20-30% performance regression). This is visible on, but I've also verified it locally. also shows a recent 35% performance regression in octane-Box2D, which is probably also related to bug 1341261.
Flags: needinfo?(mgaudet)
Investigation on Ares6/Basic showed we were missing cases like this:

    "file":"cli.js line 44 > eval",

In my original version of the patches on Bug 1341261 the support was
hidden, and got lost during the the review cleanups.

My method for ensuring 100% coverage involved a patch where I marked
all the old shared-IC attach sites as MOZ_CRASH, but I failed to redo
that analysis after review edits. Because we don't really have a way
of testing cache-attachment, this lost case got turned into a perf

After this patch goes up, I'll redo that analysis to make sure I haven't 
missed anything else. 

I have yet to confirm that we also fix the Box2D regression. Will
check after this patch goes up.

try push:
Attachment #8999976 - Flags: review?(tcampbell)
Assignee: nobody → mgaudet
Flags: needinfo?(mgaudet)
Oh, I should mention, this patch does locally restore the performance of Ares6/Basic.
Comment on attachment 8999976 [details] [diff] [review]
Add support for comparing types against null/undefined to CacheIR stubs

Review of attachment 8999976 [details] [diff] [review]:

We chatted about this a bit and I think these case should be handled by CompareIRGenerator::tryAttachNumberUndefined which should be generalized as tryAttachPrimitiveUndefined to cover {null, undefined} x {string, symbol, bool, int32, double} := false.

This also would break the DDA-Object vs undefined case if we ever changed our IC insertion order which seems not great.
Attachment #8999976 - Flags: review?(tcampbell) → review-
Also fix one missed cleanup nit from original landing
Attachment #9000050 - Flags: review?(tcampbell)
Comment on attachment 9000050 [details] [diff] [review]
Comment Compare IC attachment logic

Review of attachment 9000050 [details] [diff] [review]:


::: js/src/jit/CacheIR.cpp
@@ +4997,5 @@
>      static_assert(lhsIndex == 0 && rhsIndex == 1,
>          "Indexes relied upon by baseline inspector");
> +    ValOperandId lhsId(writer.setInputOperandId(lhsIndex));
> +    ValOperandId rhsId(writer.setInputOperandId(rhsIndex));


@@ +5013,5 @@
>              return true;
>          if (tryAttachSymbol(lhsId, rhsId))
>              return true;
> +
> +        // Handle the special case of Object compared to number/null.

Attachment #9000050 - Flags: review?(tcampbell) → review+
Comment on attachment 9000049 [details] [diff] [review]
Add PrimitiveUndefined support to CacheIR

Review of attachment 9000049 [details] [diff] [review]:

Great. Much cleaner approach

::: js/src/jit/CacheIR.cpp
@@ +5013,5 @@
>              return true;
> +        // The tag check in StrictDifferentTypes is more generic, covering more cases with
> +        // one IC, than the checks in PrimitiveUndefined, and so we put this
> +        // below it.

// These checks should come after tryAttachStrictDifferentTypes since it handles strict inequality with a more generic IC.

@@ +5021,2 @@
>          // This should come after strictDifferent types to
>          // allow it to only handle sloppy equality.

remove this comment and the whitespace above
Attachment #9000049 - Flags: review?(tcampbell) → review+
Comment on attachment 9000050 [details] [diff] [review]
Comment Compare IC attachment logic

Review of attachment 9000050 [details] [diff] [review]:

::: js/src/jit/CacheIR.cpp
@@ +5004,5 @@
> +    // - Symbol x {Primitive} is missing the cases for sloppy equality
> +    // - String has no support any primitives (Int32 support is Bug 1467907, though
> +    //   the remainder of cases are challenging)
> +    // - Boolean x Double is missing
> +    // - Object x {Primitive} are missing, though also are challenging.

Sorry.. changed my mind. Let's make this very clear:

// For sloppy equality ops, there are cases this IC does not handle:
- {Symbol} x {Null, Undefined, String, Bool, Number}.
- {String} x {Null, Undefined, Symbol, Bool, Number}. Bug 1467907 will add support for {String} x {Int32}.
- {Bool} x {Double}.
- {Object} x {String, Symbol, Bool, Number}.
Attachment #8999976 - Attachment is obsolete: true
Pushed by
Add PrimitiveUndefined support to CacheIR r=tcampbell
Comment Compare IC attachment logic r=tcampbell
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.