1523633 - Constructor comparison (or lookup?) is much slower for integer numbers than regular doubles

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[j.l.vanderzwan]

Attachment: integer vs double.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

I was benchmarking the speed of comparing constructors vs using typeof on perf.zone (see links below for final benchamark code). I noticed Firefox was performing much slower than Chrome. Investigating, I realized that the numbers I used were all integers, so I wondered if maybe internal optimizations to use 32-bit integers somehow made this much slower.

https://run.perf.zone/view/typeof-vs-constructor-string-or-number-1548684549971

https://run.perf.zone/view/typeof-vs-constructor-string-or-number-integers-only-1548686398967

Firefox 64, Ubuntu.

Actual results:

Comparing constructors for numbers known to be integer values was significantly slower in Firefox (see attached screenshots)

Expected results:

No performance regression, or at least a much smaller one, compared to regular doubles.

Comment 1

[Liviu Seplecan]

hi, I've moved this to a new component and if this isn't the right one, I guess the dev's team will move that to a suitable one.

Comment 2

[j.l.vanderzwan]

Sure! Since I only tested this in my browser I figured I should first submit it to the Firefox-bugzilla.

(also I totally get that this is probably not an big deal - how often to people look up constructors on Number objects? - but it could indicate that something else is going on that's fishy)

Comment 3

[André Bargull [:anba]]

This looks potentially wrong. If the input is an Int32, don't we need to guard against JSVAL_TYPE_INT32 instead of JSVAL_TYPE_DOUBLE?

Comment 4

[Tom S [:evilpie]]

You are correct. I think this used to work before https://hg.mozilla.org/integration/mozilla-inbound/rev/8cba4fe27807.

Comment 5

[André Bargull [:anba]]

Attachment: bug1523633.patch

Call guardIsNumber instead of guardType(..., JSVAL_TYPE_DOUBLE) for int32 values.

We could avoid the additional call to Value::isNumber by reusing primitiveType resp. now protoKey, but having an explicit call next to guardIsNumber seemed a bit nicer to me and it also matches the pattern used in, e.g. TypeOfIRGenerator::tryAttachPrimitive.

Drive-by change:

• Avoid duplicate code to retrieve the prototype object and replace RootedNativeObject proto with RootedObject proto, because we don't call any functions which require NativeObject*.

Comment 6

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

Comment on attachment 9041435 [details] [diff] [review]
bug1523633.patch

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

Good catch, nice simplification work as well :)

Comment 7

[André Bargull [:anba]]

Attachment: bug1523633.patch

Updated patch to handle the new GuardIsNumber in GetCacheIRExpectedInputType.

The additional change is needed to avoid this assertion.

Comment 8

[Jan de Mooij [:jandem]]

We should consider backporting this to beta. Still early in the cycle and it's pretty common for code to use n.toString() etc.

Comment 9

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3011d02365942df0131c2efcfd80761ee02056ab

Comment 10

[André Bargull [:anba]]

NI myself as a reminder to request backport.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96c7bf80c0f2
Use guardIsNumber in GetPropIRGenerator::tryAttachPrimitive to properly handle int32 values. r=anba

Comment 12

[j.l.vanderzwan]

Still early in the cycle and it's pretty common for code to use n.toString() etc.

Out of curiosity, could you elaborate on this? I thought I reported a pretty obscure performance regression but this kind of sounds like it affects more things than I expected.

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/96c7bf80c0f2

Comment 14

[Jan de Mooij [:jandem]]

(In reply to j.l.vanderzwan from comment #12)

Out of curiosity, could you elaborate on this? I thought I reported a pretty obscure performance regression but this kind of sounds like it affects more things than I expected.

It basically affects all property lookups of the form <int32>.something. So int32.toString(), int32.constructor, etc.

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

Not tracking for 67 as this is already fixed on mozilla-central so this is riding the trains. I'll let Liz decide if she wants to track it for 66 and request an uplift to beta.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Want to request uplift - if you think it is relatively low risk that is?
jandem (or anba) is there a way to verify the fix in nightly ?

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Pascal Chevrel:pascalc from comment #15)

Not tracking for 67 as this is already fixed on mozilla-central so this is riding the trains.

Hm I thought we still set tracking flags in that case so we don't lose track of bugs when changes are backed out?

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #16)

Want to request uplift - if you think it is relatively low risk that is?
jandem (or anba) is there a way to verify the fix in nightly ?

Should be low risk. Leaving this to anba (already needinfo'd) :)

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

(In reply to Jan de Mooij [:jandem] from comment #17)

Hm I thought we still set tracking flags in that case so we don't lose track
of bugs when changes are backed out?

I tend to do that, but it's up to the release owner. Here, I'm tracking it anyway for 66 so I'd notice.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

jandem, or steven, can you do the uplift request (if you think it is wise?) We're halfway through the beta cycle and so if you want this in 66 it should happen soon in case we see any problems as a result.

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #19)

jandem, or steven, can you do the uplift request (if you think it is wise?) We're halfway through the beta cycle and so if you want this in 66 it should happen soon in case we see any problems as a result.

It's two weeks later now so at this point we should probably just let this ride the trains.