Closed Bug 1666274 Opened 2 months ago Closed 2 months ago

Support static (rodata) ParserAtoms for wellknowns

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

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

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

The tiny parseratoms optimization (as well as the well-known parseratoms optimization) add overhead to the base content memory usage. The appropriate fix is to actually have static atom initialization for parser atoms.

  • Make all ParserAtoms "inline" in order to remove internal pointers
  • Remove the Variant type in favour of a simple tag kind to make ParserAtom constexpr
  • Split the "tiny" well-known atoms from the common property names list so that they can use the same storage as the tiny atom array.
Depends on: 1666282

Make handling of well-known names more explicit in the various interning
entry points. Different entry points can avoid these checks in different ways
so being more explicit is helpful. This also removes the AddPtr wrapper which
leads to a small speedup in practice.

In practice we always allocated clones of the string contents for non-inline
ParserAtoms which was simply an extra allocation. Any hash tables over
ParserAtoms are already using pointers so they are unaffected.

Depends on D91084

Now that all allocations are inline we can remove the tagged buffer pointer
and have it implicitly computed from this instead.

Update the atomIndex to avoid using a Variant and instead manage the type tag
explicitly. We also remove the use of mutable keyword to allow constexpr
atoms to later be stored in the .rodata section.

Add a StaticParserAtomEntry type to use for this initialization.

Depends on D91085

Use constexpr initialization for bake ParserAtomEntries for tiny well-known
atoms into the executable. This is done in a new type with a constexpr
constructor that computes the correct atom data. Some StaticStrings helper
methods must be moved to the header in order to compute constexpr values.

Since some CommonPropertyNames are already encoded in the tiny atom tables,
we must take care not to generate a duplicate atom. To achieve this we split
the FOR_EACH_COMMON_PROPERTYNAME list into two. The parser atoms for this
named tiny strings use a new initTinyStringAlias method to avoid duplicates.

Depends on D91086

Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08970bdcc4a8
Inline ParserAtomsTable::lookupForAdd into callers r=djvj
https://hg.mozilla.org/integration/autoland/rev/7302dce9d2ce
Always use inline ParserAtomEntries r=djvj
https://hg.mozilla.org/integration/autoland/rev/9c9de11d4a7e
Support constexpr initialization of ParserAtomEntry r=djvj
https://hg.mozilla.org/integration/autoland/rev/09648c7997b6
Add WellKnownParserAtoms_ROM table r=djvj

Make handling of well-known names more explicit in the various interning
entry points. Different entry points can avoid these checks in different ways
so being more explicit is helpful. This also removes the AddPtr wrapper which
leads to a small speedup in practice.

Depends on D91423

In practice we always allocated clones of the string contents for non-inline
ParserAtoms which was simply an extra allocation. Any hash tables over
ParserAtoms are already using pointers so they are unaffected.

Depends on D91424

Now that all allocations are inline we can remove the tagged buffer pointer
and have it implicitly computed from this instead.

Update the atomIndex to avoid using a Variant and instead manage the type tag
explicitly. We also remove the use of mutable keyword to allow constexpr
atoms to later be stored in the .rodata section.

Add a StaticParserAtomEntry type to use for this initialization.

Depends on D91425

Use constexpr initialization for bake ParserAtomEntries for tiny well-known
atoms into the executable. This is done in a new type with a constexpr
constructor that computes the correct atom data. Some StaticStrings helper
methods must be moved to the header in order to compute constexpr values.

Since some CommonPropertyNames are already encoded in the tiny atom tables,
we must take care not to generate a duplicate atom. To achieve this we split
the FOR_EACH_COMMON_PROPERTYNAME list into two. The parser atoms for this
named tiny strings use a new initTinyStringAlias method to avoid duplicates.

Depends on D91426

Comment on attachment 9177882 [details]
Bug 1666274 - [Beta82] Add WellKnownParserAtoms_ROM table. 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 1666282
  • 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.

Main risk is build-time failures on unusual compilers. In those cases the build may fail, or may simply run without the memory savings. This patch works for our current compilers and the Fission team relies on similar approaches elsewhere.

  • String changes made/needed: No
Attachment #9177882 - Flags: approval-mozilla-beta?
Attachment #9177879 - Flags: approval-mozilla-beta?
Attachment #9177880 - Flags: approval-mozilla-beta?
Attachment #9177881 - Flags: approval-mozilla-beta?

Comment on attachment 9177882 [details]
Bug 1666274 - [Beta82] Add WellKnownParserAtoms_ROM table. r?djvj

approved for 82.0b4

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