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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[André Bargull [:anba]]

bug 1341261, part 7 led to a performance regression in Ares-6/Basic (20-30% performance regression). This is visible on https://arewefastyet.com/#machine=29&view=breakdown&suite=ares6, but I've also verified it locally.

https://arewefastyet.com/#machine=29&view=single&suite=octane&subtest=Box2D also shows a recent 35% performance regression in octane-Box2D, which is probably also related to bug 1341261.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Add support for comparing types against null/undefined to CacheIR stubs

Investigation on Ares6/Basic showed we were missing cases like this:

  {
    "name":"Compare",
    "file":"cli.js line 44 > eval",
    "mode":0,
    "line":264,
    "column":11,
    "pc":"1051d5720",
    "lhs":{
      "type":"int32",
      "value":9
    },
    "rhs":{
      "type":"null"
    },
    "op":"ne"
  }

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
problem. 

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: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd2b82c0757726447b85a52eff9e8fa53d550e32

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Oh, I should mention, this patch does locally restore the performance of Ares6/Basic.

Comment 3

[Ted Campbell [:tcampbell]]

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.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Add PrimitiveUndefined support to CacheIR

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Comment Compare IC attachment logic

Also fix one missed cleanup nit from original landing

Comment 6

[Ted Campbell [:tcampbell]]

Comment on attachment 9000050 [details] [diff] [review]
Comment Compare IC attachment logic

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

Excellent.

::: 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.

s/number/undefined/

Comment 7

[Ted Campbell [:tcampbell]]

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

Comment 8

[Ted Campbell [: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
@@ +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}.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fde17a234256
Add PrimitiveUndefined support to CacheIR r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d191bd81bbe
Comment Compare IC attachment logic r=tcampbell

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/fde17a234256
https://hg.mozilla.org/mozilla-central/rev/4d191bd81bbe