Closed Bug 1523633 Opened 7 months ago Closed 6 months ago

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

Categories

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

64 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 + wontfix
firefox67 --- fixed

People

(Reporter: j.l.vanderzwan, Assigned: anba)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image 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.

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.

Component: Untriaged → JavaScript Engine
Product: Firefox → Core

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)

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?

Status: UNCONFIRMED → NEW
Component: JavaScript Engine → JavaScript Engine: JIT
Ever confirmed: true

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

Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attached patch bug1523633.patch (obsolete) — Splinter Review

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*.
Attachment #9041435 - Flags: review?(mgaudet)
Comment on attachment 9041435 [details] [diff] [review]
bug1523633.patch

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

Good catch, nice simplification work as well :)
Attachment #9041435 - Flags: review?(mgaudet) → review+
Attached patch bug1523633.patchSplinter Review

Updated patch to handle the new GuardIsNumber in GetCacheIRExpectedInputType.

The additional change is needed to avoid this assertion.

Attachment #9041435 - Attachment is obsolete: true
Attachment #9041809 - Flags: review+

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

Priority: -- → P1

NI myself as a reminder to request backport.

Flags: needinfo?(andrebargull)

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

Keywords: checkin-needed

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.

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

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.

Blocks: 1432168
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression

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 ?

Flags: needinfo?(jdemooij)

(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) :)

Flags: needinfo?(jdemooij)

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

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.

Flags: needinfo?(sdetar)
Flags: needinfo?(jdemooij)

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

Flags: needinfo?(sdetar)
Flags: needinfo?(jdemooij)
Flags: needinfo?(andrebargull)
You need to log in before you can comment on or make changes to this bug.