1503116 - 40% performance regression of Array.prototype.sort(cmp_fn) since Firefox63

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Alice0775 White]

Attachment: standalone testcase

+++ This bug was initially created as a clone of Bug #1502882 +++

Steps to reproduce:

Run the following fiddle or attached testcase
https://jsfiddle.net/c42vd3m9/2171/


Actual results:
40% slow on Firefox63 than Firefox62


On my poor PC;
62.0.3: around 28-30ms
63.0: around 41-42ms
64b4:around 43-44ms
65.0a1: around 44-45ms



Regression Window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8a5504fe1af59fa5b6fa576f6ba0a23b60fbb00d&tochange=2f962180068b5d353f3cb1d1b084382535067cd1

Regressed by: Bug 1341261

Comment 1

[Jan de Mooij [:jandem]]

NI per comment 0.

Comment 2

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

Definitely an accurate bisection. Here's a little more of the bisection (done in the shell. PS: Really like the test case attached as a clever way to do minimal browser testing). 

The test case contains two comparators, str_cmp_full, and str_cmp_first. Noticing that I was also seeing a performance issue with str_cmp_full, I first focused on that one, however the root cause is the same. 

After confusing myself into not trying the obvious, I finally tried creating a CACHEIR_LOG. Using CACHEIR_LOGS on [Part 6] of the patch series in that push produces 5.2GBs of CacheIR logs... which suggests we're missing something, just a bit. 

Seems we're not attaching any stub for string string comparison: 

    {
      "name":"Compare",
      "file":"/Users/mgaudet/bugs/1503116/benchmark.js",
      "mode":0,
      "line":12,
      "column":4,
      "pc":"10b789dab",
      "lhs":{
        "type":"string",
        "value":"15"
      },
      "rhs":{
        "type":"string",
        "value":"15"
      }
    },

In the case of str_cmp_first the values are just 1-element strings.

Comment 3

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

(I think we get so much logging because [Part 6] precedes the landing of Bug 1448039)

Comment 4

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

While I think the quick fix here would be to implement an IC for the relational ops, I am a little concerned something curious happened with the fix to Bug 1448039; 

I still get a 5GB CacheIR log on yesterday's central, and looking at the last million lines of that log only two PCs show up: 

    tail -n 1000000 cache35500.json | grep pc  | sort | uniq -c
    29411   "pc":"1042c4fca",
    29412   "pc":"1042c4fe3",

which suggests to me the root issue in Bug 1448039 is still there. 

I almost wonder if the failure to go generic is sufficient to describe the performance issues in this bug, as the old SharedICs also had no support for String {rel. op} String. 

ni? :khyperia about the seeming lack of transition to Generic mode. 

I'm going to look into what it would take to write an IC to handle the relational operations on strings.

Comment 5

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

Dropping ni? as I am sufficiently curious about what's going on that I'll look at this shortly anyhow.

Comment 6

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

(Note to self: I suspect bug is in Ion state management)

Comment 7

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

Attachment: Bug 1503116: Templatize CacheIR IC attachment mechanism in Ion r?khyperia

This also, circumstantially, cleans up an ugly bug in the attachment logic
in the Binary and Compare IR generators where we would never update the
IC state.

Comment 8

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

Performance on this testcase is slightly better than the baseline with this patch.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c15b6ab74cd
Templatize CacheIR IC attachment mechanism in Ion r=khyperia

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/8c15b6ab74cd