1664312 - Optimize length-1 strings in js::ParserAtoms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Ted Campbell [:tcampbell]]

As shown in Bug 1663665, there are some regressions attributable to the ParserAtoms code landing. There are a few causes, but one that is pronounced in the raptor-tp6-netflix-cold case is that ParserAtoms do not have any fast-path for length-1/2 strings. In the normal atomize path we use the StaticStrings mechanism and should do the same.

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: Bug 1664312 - Rename ParserAtomsTable::internUtf8 length arg to nbyte

In this code we often use 'length' to refer to number of char16_t and this
can be a little confusing with UTF-8 so instead use 'nbytes' as the argument
until we establish what length of the inflated sequence is.

Depends on D90148

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Bug 1664312 - Reorder ParserAtomsTable methods

Depends on D90149

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Bug 1664312 - Inline ParserAtomsTable::lookupOrInternChar16Seq into callers

This lets us specialize how and when well-known lookups are done.

Depends on D90150

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Bug 1664312 - Use mozilla::Variant in js::frontend::ParserAtomEntry

Later patches will add more types of known-atoms and MaybeOneOf has a limit
on the number of types.

Depends on D90151

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: Bug 1664312 - Optimize tiny atoms in ParserAtomsTable

This add a similar optimization as js::StaticStrings for tiny strings to the
ParserAtoms mechanism. This includes (some) length 1 and length 2 atoms for
fast lookup. This is effective for large minified JS files and can speed up
syntax-parsing by up to 20% in some cases. We extend the atomIndex_ Variant
to have new StaticParserString{1,2} types which we use to in toJSAtom to
quickly translate to the corresponding StaticStrings.

Depends on D90152

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8867b79ee8ea
Rename ParserAtomsTable::internUtf8 length arg to nbyte r=djvj
https://hg.mozilla.org/integration/autoland/rev/7c31676a20cd
Reorder ParserAtomsTable methods r=djvj
https://hg.mozilla.org/integration/autoland/rev/210475dc1429
Inline ParserAtomsTable::lookupOrInternChar16Seq into callers r=djvj
https://hg.mozilla.org/integration/autoland/rev/15482e97aa4a
Use mozilla::Variant in js::frontend::ParserAtomEntry r=djvj
https://hg.mozilla.org/integration/autoland/rev/71d14ede1fcb
Optimize tiny atoms in ParserAtomsTable r=djvj

Comment 7

[Cosmin Sabou [:CosminS]]

Backed out for valgrind bustages about ParserAtom.cpp.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&revision=71d14ede1fcb8eaf54659c37226dd78eb23ed6ac&searchStr=valgrind

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315868064&repo=autoland&lineNumber=66784

Backout link: https://hg.mozilla.org/integration/autoland/rev/b2d8201169eaddec051abaae7685656b2bd2a3fb

Comment 8

[Ted Campbell [:tcampbell]]

Leaking the ParserAtomEntrys used for tiny strings. Moved to UniquePtr and verified valgrind looks good (for jsshell) again. This was a shutdown leak only since they data lives as long as the main JSRuntime, but still should be fixed.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8e17781c000
Rename ParserAtomsTable::internUtf8 length arg to nbyte r=djvj
https://hg.mozilla.org/integration/autoland/rev/36a190a3d381
Reorder ParserAtomsTable methods r=djvj
https://hg.mozilla.org/integration/autoland/rev/13ace5d469df
Inline ParserAtomsTable::lookupOrInternChar16Seq into callers r=djvj
https://hg.mozilla.org/integration/autoland/rev/dc0371757070
Use mozilla::Variant in js::frontend::ParserAtomEntry r=djvj
https://hg.mozilla.org/integration/autoland/rev/c5bf76bc593f
Optimize tiny atoms in ParserAtomsTable r=djvj

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b8e17781c000
https://hg.mozilla.org/mozilla-central/rev/36a190a3d381
https://hg.mozilla.org/mozilla-central/rev/13ace5d469df
https://hg.mozilla.org/mozilla-central/rev/dc0371757070
https://hg.mozilla.org/mozilla-central/rev/c5bf76bc593f

Comment 11

[Florin Strugariu [:Bebe]]

== Change summary for alert #27061 (as of Fri, 25 Sep 2020 12:38:40 GMT) ==

Improvements:

4% google-maps loadtime android-hw-g5-7-0-arm7-api-16-shippable opt warm 1,190.19 -> 1,137.58
2% google-maps PerceptualSpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt warm 1,021.76 -> 999.33
2% google-maps fcp android-hw-g5-7-0-arm7-api-16-shippable opt warm 930.22 -> 910.08
2% google-maps FirstVisualChange android-hw-g5-7-0-arm7-api-16-shippable opt warm 940.81 -> 921.00
2% google-maps SpeedIndex android-hw-g5-7-0-arm7-api-16-shippable opt warm 1,028.48 -> 1,007.58
2% google-maps fcp android-hw-g5-7-0-arm7-api-16-shippable opt warm 930.75 -> 914.67

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