Closed Bug 1687428 Opened 4 months ago Closed 3 months ago

Use TaggedParserAtomIndex everywhere and remove ParserAtomEntry::index_ field

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox87 --- fixed

People

(Reporter: arai, Assigned: arai)

References

(Blocks 1 open bug)

Details

Attachments

(37 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

ParserAtomEntry contains index_ field, to get TaggedParserAtomIndex from ParserAtom pointer.
this is used while converting frontend-internal data to stencil, and also used while stencil instantiation/XDR.

If we can replace all ParserAtom* fields/variables with TaggedParserAtomIndex, we get:

  • on-memory size reduction, especially in ParseNode
  • single ParserAtomEntry instance can be shared between multiple tables
See Also: → 1687094

The plan is the following:

  1. rewrite each ParserAtom consumer class to receive/return TaggedParserAtomIndex instead of ParserAtom/ParserName
    • this happens gradually and the intermediate state may be somewhat unoptimal
    • at the end of this stage, ParserAtomEntry::toIndex should be called only after invoking ParserAtomsTable methods
  2. make ParserAtomsTable return TaggedParserAtomIndex instead of ParserAtom/ParserName
    • after this stage, there's only "TaggedParserAtomIndex -> ParserAtom" conversion, and no "ParserAtom -> TaggedParserAtomIndex" conversion
  3. remove ParserAtomEntry::index_ field and ParserAtomEntry::toIndex
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Priority: P3 → P1

This seems to work. I'll look into bug 1687094 after this.

Blocks: 1687094
See Also: 1687094
No longer blocks: stencil-backlog
Blocks: 1687634

With the raptor yahoo-mail, mail_app_es6_09* JS file's XDR,
my local patch stack reduces the size of atom table part from 1,977,500 bytes to 1,776,884 bytes,
so 200,616 bytes (10%) reduction (~50000 atoms * 4 bytes per each index)

Also this enables us to dedup ParserAtomEntry inside XDR, so we can expect further reduction :)

To gradually replace all consumers, provide both ParserAtom/ParserName accessor
and TaggedParserAtomIndex accessor.

ParserAtom/ParserName accessor will be removed later, and TaggedParserAtomIndex
accessor's Index suffix will be removed later.

Also add ParserAtomsTable::markUsedByStencil method to hide ParserAtom pointer.

Depends on D102533

Given TaggedParserAtomIndex::null() isn't well-known "null",
we should split the namespace.

Also, now that there's no risk of name collision, populate all well-known
indices.

Depends on D102534

Added TrivialTaggedParserAtomIndex to use TaggedParserAtomIndex in collection
that requires trivial type.

Also move TaggedParserAtomIndexHasher into its own header,
with TrivialTaggedParserAtomIndexHasher, to share between multiple consumers.

Depends on D102535

Because of VectorPool requirement, the element needs to be pointer size,
so add TrivialPtrSizeTaggedParserAtomIndex, that uses uintptr_t instead of
uint32_t.

Depends on D102536

IsEscapeFreeStringLiteral is split to each useStrict/useAsm case, in order to
avoid accessing ParserAtom content.

Depends on D102543

Now NameNode and LoopControlStatement uses TaggedParserAtomIndex in its field.

ParseNode::dump is rewritten to receive ParserBase, to get ParserAtomsTable
access, in order to dump not-well-known atom content.
It also works without ParserAtomsTable (in case ParseNode::dump is directly
called from debugger), but in that case not-well-known is dumped as index.

Depends on D102544

Also renames TaggedParserAtomIndex-specific accessor to remove Index suffix.

TokenStreamAnyChars::currentName receives ParserAtomsTable to get
ParserAtom, but this change is temporary and will be reverted when removing
ParserAtom/ParserName from TokenStream.

Depends on D102548

There's inconsistency around compilationState_.getParserAtomAt vs
compilationState_.parserAtoms.getParserAtom.
This will be solved later.

Also removed unused/not-existent functions liftParserAtomToJSAtom and
newFunction.

Depends on D102552

As a preparation to remove index_ field from ParserAtomEntry,
ParserAtomsTable should maintain a map from ParserAtomEntry pointer to
TaggedParserAtomIndex.

Depends on D102558

ParserAtomsTable::concatAtoms still receives ParserAtom* range, because we need
character access, and InflatedChar16Sequence needs ParserAtom* range.

Depends on D102559

As a preparation to remove index_ field, rewrite its consumer to use different
way to get index.

Also, ParserAtomEntry::{toJSAtom, instantiate} receive TaggedParserAtomIndex,
to avoid using its field, but with an assertion for now.
The assertion will be removed later.

In InstantiateMarkedAtoms and XDRParserAtomTable, the atom's index matches the
item's index inside the span, and isUsedByStencil is set only for
not-well-known atoms, so we can just check isUsedByStencil and then use
the index inside the span.

Depends on D102562

ParserAtomEntry::index_ field is used also to distinguish well-known/static.
We need separate flag, in order to remove the field.

Depends on D102564

Because CompilationState.parserAtoms.getParserAtom provides the same thing,
removed CompilationState::getParserAtomAt.

Depends on D102566

Blocks: 1688534
Blocks: 1660275
Blocks: 1689334
Blocks: 1689434
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/a295d374f63c
Part 1: Add TaggedParserAtomIndex-specific accessor to ParserNode and FunctionBox. r=nbp
https://hg.mozilla.org/integration/autoland/rev/cf923036a361
Part 2: Split tagged index raw data accessor into mutable and immutable. r=nbp
https://hg.mozilla.org/integration/autoland/rev/21dae3d1ea01
Part 3: Use TaggedParserAtomIndex in frontend::GCThingList. r=nbp
https://hg.mozilla.org/integration/autoland/rev/f11873f0827c
Part 4: Move well-known TaggedParserAtomIndex methods into dedicate class. r=nbp
https://hg.mozilla.org/integration/autoland/rev/721a764c88ea
Part 5: Use TaggedParserAtomIndex in RecyclableNameMap. r=nbp
https://hg.mozilla.org/integration/autoland/rev/48bc889e7f9b
Part 6: Use TaggedParserAtomIndex in AtomVector. r=nbp
https://hg.mozilla.org/integration/autoland/rev/178975ab315b
Part 7: Use TaggedParserAtomIndex in EmitterScope. r=nbp
https://hg.mozilla.org/integration/autoland/rev/02c07b673b6a
Part 8: Use TaggedParserAtomIndex in LabelControl. r=nbp
https://hg.mozilla.org/integration/autoland/rev/2b773e770cf6
Part 9: Use TaggedParserAtomIndex in BytecodeEmitter. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d581c6939b6e
Part 10: Use TaggedParserAtomIndex in emitter helpers. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d53977dc05a6
Part 11: Use TaggedParserAtomIndex in ModuleBuilder. r=nbp
https://hg.mozilla.org/integration/autoland/rev/a3cbfc45580b
Part 12: Use TaggedParserAtomIndex in AsmJS. r=nbp
https://hg.mozilla.org/integration/autoland/rev/14fc717dea9a
Part 13: Remove ParserAtom/ParserName accessor from ParseNode. r=nbp
https://hg.mozilla.org/integration/autoland/rev/b55652713ba2
Part 14: Remove ParserAtom/ParserName from ParseNode and ParseHandler. r=nbp
https://hg.mozilla.org/integration/autoland/rev/08e996759dd9
Part 15: Rename TaggedParserAtomIndex-specific accessor in ParseNode. r=nbp
https://hg.mozilla.org/integration/autoland/rev/e53c556bba8c
Part 16: Remove ParserAtom/ParserName from SharedContext/FunctionBox. r=nbp
https://hg.mozilla.org/integration/autoland/rev/c2f48e868180
Part 17: Rename TaggedParserAtomIndex-specific accessor in FunctionBox. r=nbp
https://hg.mozilla.org/integration/autoland/rev/e3b13445f55a
Part 18: Remove ParserAtom/ParserName from Token. r=nbp
https://hg.mozilla.org/integration/autoland/rev/b722d3036840
Part 19: Remove ParserAtom/ParserName from TokenStream methods. r=nbp
https://hg.mozilla.org/integration/autoland/rev/0810505263e6
Part 20: Remove ParserAtom/ParserName from ParseContext. r=nbp
https://hg.mozilla.org/integration/autoland/rev/48bf6019c703
Part 21: Remove unused liftParserAtomToJSAtom method from EitherParser. r=nbp
https://hg.mozilla.org/integration/autoland/rev/73ea6560f99d
Part 22: Remove ParserAtom/ParserName from Parser. r=nbp
https://hg.mozilla.org/integration/autoland/rev/b8318cf13b27
Part 23: Remove ParserAtom/ParserName from RealmInstrumentation. r=nbp
https://hg.mozilla.org/integration/autoland/rev/1b8f396bdcb5
Part 24: Remove ParserAtom/ParserName return type from StringBuffer. r=nbp
https://hg.mozilla.org/integration/autoland/rev/08b75ff379d9
Part 25: Remove ParserAtom/ParserName from jsnum. r=nbp
https://hg.mozilla.org/integration/autoland/rev/fc49babba848
Part 26: Remove ParserAtom/ParserName from Frontend2. r=nbp
https://hg.mozilla.org/integration/autoland/rev/b89b5f731712
Part 27: Remove remaining unnecessary ParserAtomEntry::toIndex callsite. r=nbp
https://hg.mozilla.org/integration/autoland/rev/2fa38f16f81a
Part 28: Store TaggedParserAtomIndex in ParserAtomsTable. r=nbp
https://hg.mozilla.org/integration/autoland/rev/21b2349db370
Part 29: Return TaggedParserAtomIndex from ParserAtomsTable::concatAtoms. r=nbp
https://hg.mozilla.org/integration/autoland/rev/5d9fb3328608
Part 30: Return TaggedParserAtomIndex from ParserAtomsTable::intern*. r=nbp
https://hg.mozilla.org/integration/autoland/rev/cbfc7ec206b4
Part 31: Return TaggedParserAtomIndex from WellKnownParserAtoms::lookup*. r=nbp
https://hg.mozilla.org/integration/autoland/rev/7e28576fa2f3
Part 32: Hide ParserAtomEntry index accessors. r=nbp
https://hg.mozilla.org/integration/autoland/rev/bb45134f6099
Part 33: Remove most of ParserAtomEntry index accessors. r=nbp
https://hg.mozilla.org/integration/autoland/rev/ff9cadc682f7
Part 34: Add WellKnownOrStaticFlag to ParserAtomEntry. r=nbp
https://hg.mozilla.org/integration/autoland/rev/c1189f833fbc
Part 35: Remove ParserAtomEntry::index_ field. r=nbp
https://hg.mozilla.org/integration/autoland/rev/ba478cc96ac8
Part 36: Remove CompilationState::getParserAtomAt. r=nbp
https://hg.mozilla.org/integration/autoland/rev/44155b6558c5
Part 37: Move getParserAtom into ParserAtomToPrintableString. r=nbp

https://hg.mozilla.org/mozilla-central/rev/a295d374f63c
https://hg.mozilla.org/mozilla-central/rev/cf923036a361
https://hg.mozilla.org/mozilla-central/rev/21dae3d1ea01
https://hg.mozilla.org/mozilla-central/rev/f11873f0827c
https://hg.mozilla.org/mozilla-central/rev/721a764c88ea
https://hg.mozilla.org/mozilla-central/rev/48bc889e7f9b
https://hg.mozilla.org/mozilla-central/rev/178975ab315b
https://hg.mozilla.org/mozilla-central/rev/02c07b673b6a
https://hg.mozilla.org/mozilla-central/rev/2b773e770cf6
https://hg.mozilla.org/mozilla-central/rev/d581c6939b6e
https://hg.mozilla.org/mozilla-central/rev/d53977dc05a6
https://hg.mozilla.org/mozilla-central/rev/a3cbfc45580b
https://hg.mozilla.org/mozilla-central/rev/14fc717dea9a
https://hg.mozilla.org/mozilla-central/rev/b55652713ba2
https://hg.mozilla.org/mozilla-central/rev/08e996759dd9
https://hg.mozilla.org/mozilla-central/rev/e53c556bba8c
https://hg.mozilla.org/mozilla-central/rev/c2f48e868180
https://hg.mozilla.org/mozilla-central/rev/e3b13445f55a
https://hg.mozilla.org/mozilla-central/rev/b722d3036840
https://hg.mozilla.org/mozilla-central/rev/0810505263e6
https://hg.mozilla.org/mozilla-central/rev/48bf6019c703
https://hg.mozilla.org/mozilla-central/rev/73ea6560f99d
https://hg.mozilla.org/mozilla-central/rev/b8318cf13b27
https://hg.mozilla.org/mozilla-central/rev/1b8f396bdcb5
https://hg.mozilla.org/mozilla-central/rev/08b75ff379d9
https://hg.mozilla.org/mozilla-central/rev/fc49babba848
https://hg.mozilla.org/mozilla-central/rev/b89b5f731712
https://hg.mozilla.org/mozilla-central/rev/2fa38f16f81a
https://hg.mozilla.org/mozilla-central/rev/21b2349db370
https://hg.mozilla.org/mozilla-central/rev/5d9fb3328608
https://hg.mozilla.org/mozilla-central/rev/cbfc7ec206b4
https://hg.mozilla.org/mozilla-central/rev/7e28576fa2f3
https://hg.mozilla.org/mozilla-central/rev/bb45134f6099
https://hg.mozilla.org/mozilla-central/rev/ff9cadc682f7
https://hg.mozilla.org/mozilla-central/rev/c1189f833fbc
https://hg.mozilla.org/mozilla-central/rev/ba478cc96ac8
https://hg.mozilla.org/mozilla-central/rev/44155b6558c5

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
You need to log in before you can comment on or make changes to this bug.