Closed Bug 1666282 Opened 1 year ago Closed 1 year ago

Cleanup ParserAtomsTable::concatAtoms

Categories

(Core :: JavaScript Engine, task, P2)

task

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox82 --- fixed
firefox83 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.

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.

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

Blocks: 1666274
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
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch

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.

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 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
Attachment #9177878 - Flags: approval-mozilla-beta?
Attachment #9177877 - Flags: approval-mozilla-beta?

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

approved for 82.0b4

Attachment #9177878 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9177877 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.