1666282 - Cleanup ParserAtomsTable::concatAtoms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Ted Campbell [:tcampbell]]

The implementation for ParserAtomsTable::concatAtoms relies on a lot of implementation details that make it harder to tune. Instead we can fast-path the common latin1 short string case (and handle well-known atoms) and for everything else can teach the InflatedChar16Sequence to iterate directly over the sources. Concatenating ParserAtoms is not a hot-path in the parser so favouring simplicity seems better here.

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: Bug 1666282 - Assert ParserAtomsTable::concatAtoms is passed multiple atoms. r?djvj

The FoldAdd code will sometimes accumulate a list of only one atom to
concatenate. This should instead fast-path and skip the call to concatAtoms
to be less surprising.

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Bug 1666282 - Simplify ParserAtomsTable::concatAtoms. r?djvj

Add support for InflatedChar16Sequence to directly traverse the list of
ParserAtoms to allow reusing code for most of the more complicated cases. For
results that are small and Latin1, we always concat in a stack-allocated
buffer and use the normal internLatin1 path to handle all tiny and well-known
atom cases. The fall-through case does not have to worry about tiny or
well-known atoms as a result.

In the parser, concat is used for combining string literals (although maybe
we should prefer ropes here) or for adding prefixes like "get " on certain
method names.

Depends on D90886

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58e64de2d1d8
Assert ParserAtomsTable::concatAtoms is passed multiple atoms. r=djvj

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/491b1fb5bac0
Simplify ParserAtomsTable::concatAtoms. r=djvj

Comment 5

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/58e64de2d1d8
https://hg.mozilla.org/mozilla-central/rev/491b1fb5bac0

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: Bug 1666282 - [Beta82] Assert ParserAtomsTable::concatAtoms is passed multiple atoms. r?djvj

The FoldAdd code will sometimes accumulate a list of only one atom to
concatenate. This should instead fast-path and skip the call to concatAtoms
to be less surprising.

Comment 7

[Ted Campbell [:tcampbell]]

Attachment: Bug 1666282 - [Beta82] Simplify ParserAtomsTable::concatAtoms. r?djvj

Add support for InflatedChar16Sequence to directly traverse the list of
ParserAtoms to allow reusing code for most of the more complicated cases. For
results that are small and Latin1, we always concat in a stack-allocated
buffer and use the normal internLatin1 path to handle all tiny and well-known
atom cases. The fall-through case does not have to worry about tiny or
well-known atoms as a result.

In the parser, concat is used for combining string literals (although maybe
we should prefer ropes here) or for adding prefixes like "get " on certain
method names.

Depends on D91422

Comment 8

[Ted Campbell [:tcampbell]]

Comment on attachment 9177878 [details]
Bug 1666282 - [Beta82] Simplify ParserAtomsTable::concatAtoms. r?djvj

Beta/Release Uplift Approval Request

• User impact if declined: Regression reported in Bug 1666237.
  This is a 150kB memory overhead per content process. For people dog-fooding Fission this can be significant.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1666274
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): There are targeted unit tests and this is a very hot path that nightly is now running.
• String changes made/needed: No

Comment 9

[Julien Cristau [:jcristau] (away, back August 16)]

Comment on attachment 9177878 [details]
Bug 1666282 - [Beta82] Simplify ParserAtomsTable::concatAtoms. r?djvj

approved for 82.0b4

Comment 10

[Julien Cristau [:jcristau] (away, back August 16)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e2740e946b48
https://hg.mozilla.org/releases/mozilla-beta/rev/9dd6a9f2574f