1644425 - Math.{min,max} optimization slows down ToLength

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

The self-hosted ToLength function does this:

    return std_Math_min(v, 0x1fffffffffffff);

Before bug 1643680 this used to return int32 but the CacheIR optimization will see there's a double argument and we return a double. That in turn slows down some code elsewhere.

I'm not sure how to fix this. Maybe this is a common pattern and the CacheIR IR generator should try to convert the result to int32 if there are both int32 and double arguments and the result is an int32...

Comment 1

[André Bargull [:anba]]

Similar previous issues: Bug 1042823 and bug 1356207.

Comment 2

[Jan de Mooij [:jandem]]

Another approach is to make ToLength an intrinsic and optimize the Int32 case in the JITs in terms of Math.max.

Comment 3

[Jan de Mooij [:jandem]]

André, does comment 2 sound okay to you? I see we mostly call ToLength on the result of a .length property lookup so for those optimizing the Int32 case would be fine.

Comment 4

[André Bargull [:anba]]

I think that sounds reasonable as a short-term mean to fix the regression. If a similar case ever occurs outside of self-hosted code, we can re-evaluate at that point of time.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1644425 part 1 - Replace ToLength function in self-hosted code with an intrinsic. r?anba!

This fixes a regression from bug 1643680 because the code relied on Math.min/max
with int32 and double arguments returning an int32 when possible. Porting this
to C++ should make this less brittle.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1644425 part 2 - Optimize ToLength intrinsic in Ion/CacheIR/Warp. r?anba!

For now this only optimizes the int32-argument case.

Depends on D78917

Comment 7

[André Bargull [:anba]]

I did take another look at the current ToLength call sites and two callers may benefit from supporting Double -> Int32:

• String_pad (helper for String.prototype.padStart and String.prototype.padEnd): ToLength is called on the input maxLength argument, which may be a Double when users compute it through division.
• Array.from also calls ToLength, that code path is taken for Array.from({length}), which I've seen some users to prefer to create a dense array. length could also be Double-typed in this case.

We should monitor if there are any performance regression reports for these functions.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a defect, but please change it back in case of error.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1643680

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74c3e27d643e
part 1 - Replace ToLength function in self-hosted code with an intrinsic. r=anba
https://hg.mozilla.org/integration/autoland/rev/834f42351b0f
part 2 - Optimize ToLength intrinsic in Ion/CacheIR/Warp. r=anba

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/74c3e27d643e
https://hg.mozilla.org/mozilla-central/rev/834f42351b0f

Comment 12

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #26215 (as of Mon, 15 Jun 2020 03:14:05 GMT) ==

Regressions:

36% tsvg_static macosx1014-64-shippable opt e10s stylo 43.14 -> 58.86
28% tsvg_static windows10-64-shippable opt e10s stylo 37.94 -> 48.64
25% tsvg_static windows10-64-shippable-qr opt e10s stylo 40.34 -> 50.51
19% tsvg_static windows7-32-shippable opt e10s stylo 44.98 -> 53.55
18% tsvg_static linux64-shippable opt e10s stylo 45.15 -> 53.11
7% tsvgr_opacity linux64-shippable opt e10s stylo 146.68 -> 156.38

Improvements:

9% tsvg_static linux64-shippable-qr opt e10s stylo 40.30 -> 36.86
8% dromaeo_css windows10-64-shippable-qr opt e10s stylo 15,672.29 -> 16,932.82
7% dromaeo_css windows7-32-shippable opt e10s stylo 16,955.75 -> 18,158.21
7% dromaeo_css linux64-shippable-qr opt e10s stylo 16,984.61 -> 18,174.20
7% dromaeo_css windows10-64-shippable opt e10s stylo 15,856.18 -> 16,945.96
7% dromaeo_css windows7-32-shippable opt e10s stylo 16,969.51 -> 18,107.79
7% dromaeo_css linux64-shippable opt e10s stylo 17,323.37 -> 18,463.42

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26215

Comment 13

[Florin Strugariu [:Bebe]]

== Change summary for alert #26189 (as of Fri, 12 Jun 2020 07:34:57 GMT) ==

Improvements:

4% raptor-ares6-firefox linux64-shippable opt 45.58 -> 43.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26189