Cleanup ParserAtomsTable::concatAtoms
Categories
(Core :: JavaScript Engine, task, P2)
Tracking
()
People
(Reporter: tcampbell, Assigned: tcampbell)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details | Review |
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.
Assignee | ||
Comment 1•2 years ago
|
||
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.
Assignee | ||
Comment 2•2 years ago
|
||
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
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/58e64de2d1d8 Assert ParserAtomsTable::concatAtoms is passed multiple atoms. r=djvj
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/491b1fb5bac0 Simplify ParserAtomsTable::concatAtoms. r=djvj
Comment 5•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/58e64de2d1d8
https://hg.mozilla.org/mozilla-central/rev/491b1fb5bac0
Assignee | ||
Comment 6•2 years ago
|
||
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.
Assignee | ||
Comment 7•2 years ago
|
||
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
Assignee | ||
Comment 8•2 years ago
|
||
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
Assignee | ||
Updated•2 years ago
|
Comment 9•2 years ago
|
||
Comment on attachment 9177878 [details]
Bug 1666282 - [Beta82] Simplify ParserAtomsTable::concatAtoms. r?djvj
approved for 82.0b4
Updated•2 years ago
|
Comment 10•2 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e2740e946b48
https://hg.mozilla.org/releases/mozilla-beta/rev/9dd6a9f2574f
Description
•