Closed Bug 1721413 Opened 4 months ago Closed 2 months ago

Use JSString instead of JSAtom for string literals during instantiation

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
94 Branch
Tracking Status
firefox94 --- fixed

People

(Reporter: arai, Assigned: arai)

References

(Blocks 2 open bugs)

Details

Attachments

(6 files)

Currently all strings including identifiers, properties, string literals are converted into JSAtom during instantiation step.

but at lease some reason for using JSAtom there were from old frontend-side, and now that frontend uses ParserAtom instead, there may be chance that we can use JSString for some case, so that instantiation step takes less time, by skipping atomization lookup and some lock.

some remaining reasons for using JSAtom:

  • identifier, for scope handling
  • deduplication
  • runtime comparison (?)

in ScriptPreloader usage, there are 400 literals with 100+ characters, and the longer literals are mostly SQL,
the longest one has 3039 chars.
given that we use other string type when we concatenate strings, we can use other JSString subclass (e.g. JSExternalString) for raw string literals as well.

also, for ScriptPreloader usage, we expect the XDR buffer be alive during the entire process lifetime, and using JSExternalString is possible option here.

Places that touches JSAtom entry in JSScript gcthings:

https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/shell/js.cpp#3578
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/vm/JSScript.cpp#298-309
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/vm/JSScript.cpp#545-555
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/vm/JSScript.cpp#4298-4306
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/vm/JSScript.h#2087-2103
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/jit/BaselineCodeGen.cpp#972-973
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/jit/BaselineCodeGen.cpp#1014-1020
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/jit/BaselineCodeGen.cpp#1072
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/jit/BaselineCodeGen.cpp#2485-2493
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/vm/BytecodeLocation-inl.h#27
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/vm/BytecodeUtil.cpp#1469-1479
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/vm/BytecodeUtil.cpp#2204-2206
https://searchfox.org/mozilla-central/rev/e082df56bbfeaff0f388e7da9da401ff414df18f/js/src/vm/Interpreter.cpp#3404-3405

in ScriptPreloader usage:

  • 532 calls for InstantiateAtoms during startup
  • 106149 calls for AtomizeChars
  • 26571 (25%) atoms are used only as binding
  • 35088 (33%) atoms are used only as property name
  • 27426 (26%) atoms are used only as string literal

(In reply to Tooru Fujisawa [:arai] from comment #3)

in ScriptPreloader usage:

  • 532 calls for InstantiateAtoms during startup
  • 106149 calls for AtomizeChars

during Firefox startup, with profiler, ~30ms is taken inside AtomizeChars for them.

in JS shell, in single realm.
calling AtomizeChars for all of them takes 16 ms,
and calling JS_NewStringCopyN/JS_NewUCStringCopyN for all of them takes 6ms.

applying this for string literals there could reduce the startup time by up to 5ms.

(In reply to Tooru Fujisawa [:arai] from comment #3)

  • 27426 (26%) atoms are used only as string literal

1791783 chars are atomized in total, and
767539 chars (43%) there are used only as string literal.

(In reply to Tooru Fujisawa [:arai] from comment #4)

in JS shell, in single realm.
calling AtomizeChars for all of them takes 16 ms,
and calling JS_NewStringCopyN/JS_NewUCStringCopyN for all of them takes 6ms.

calling JS_NewStringCopyN/JS_NewUCStringCopyN for string literal case takes 12ms

applying this for ScriptPreloader could reduce the startup time by up to 7.5ms.

If we use JSExternalString, we need Latin1Char flavor

See Also: → 1576076

then, if we use JSExternalString with draft patch for bug 1576076, the same takes 10ms,
and applying that for ScriptPreloader could reduce the startup time by up to 11.5ms

JSString cannot be freely moved across zones, in contrast to JSAtom, and that hits self-hosted functions cloning code.
although we can workaround it here, bug 1688794 will simplify this situation, so we can wait for it.

Depends on: 1688794

other note:

  • currently JSScript gcthings doesn't support strings in nursery, so strings needs to be allocted in tenured heap, when:
    • instantiating
    • cloning
    • XDR decoding
Blocks: 1721120

things to note/consider:

  • should we use non-atom linear-string for all string literal?
    • should we add minimal length requirement?
  • perf impact from copying strings across zones
  • strings in self-hosting stencil cannot use non-atom string, given there's no zone

Collect whether the ParserAtom is used only for string literal or not.
BytecodeEmitter::emitStringOp is specialized in later patch.

Depends on D122809

JSScript::getString will be specialized for string case in later patch.

Depends on D122810

bug 1688788 needs to be fixed before this, because of legacy XDR assumes all strings in script gcthings are Atom

Depends on: 1688788
Summary: Investigate using JSString instead of JSAtom for string literals during instantiation → Use JSString instead of JSAtom for string literals during instantiation
Assignee: nobody → arai.unmht
Attachment #9236575 - Attachment description: WIP: Bug 1721413 - Part 0: Remove unused field from PrivateOpEmitter. r?tcampbell! → Bug 1721413 - Part 0: Remove unused field from PrivateOpEmitter. r?tcampbell!
Status: NEW → ASSIGNED
Attachment #9236576 - Attachment description: WIP: Bug 1721413 - Part 1: Collect usage for ParserAtom during compilation. r?tcampbell! → Bug 1721413 - Part 1: Collect usage for ParserAtom during compilation. r?tcampbell!
Attachment #9236578 - Attachment description: WIP: Bug 1721413 - Part 2: Support non-atom string in JSScript gcthings. r?tcampbell! → Bug 1721413 - Part 2: Support non-atom string in JSScript gcthings. r?tcampbell!
Attachment #9236579 - Attachment description: WIP: Bug 1721413 - Part 3: Support non-atom string in JSOp::String operand. r?tcampbell! → Bug 1721413 - Part 3: Support non-atom string in JSOp::String operand. r?tcampbell!
Attachment #9236580 - Attachment description: WIP: Bug 1721413 - Part 4: Use non-atom string if the ParserAtom is used only in JSOp::String operand or object literal's property value. r?tcampbell! → Bug 1721413 - Part 4: Use non-atom string if the ParserAtom is used only in JSOp::String operand or object literal's property value. r?tcampbell!
Attachment #9236576 - Attachment description: Bug 1721413 - Part 1: Collect usage for ParserAtom during compilation. r?tcampbell! → Bug 1721413 - Part 1: Collect atomization information for ParserAtom during compilation. r?tcampbell!
Blocks: 1687973
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/78c378e9d9d2
Part 0: Remove unused field from PrivateOpEmitter. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/8bbbcc59e36b
Part 1: Collect atomization information for ParserAtom during compilation. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/114dc6e44153
Part 2: Support non-atom string in JSScript gcthings. r=tcampbell,jandem
https://hg.mozilla.org/integration/autoland/rev/06561b972cc8
Part 3: Support non-atom string in JSOp::String operand. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/18f4294ec0a2
Part 4: Use non-atom string if the ParserAtom is used only in JSOp::String operand or object literal's property value. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/ba3814055b37
Part 5 - Always use JSAtom for short latin1 strings. r=tcampbell

== Change summary for alert #31486 (as of Fri, 24 Sep 2021 09:47:54 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
1% Base Content JS windows10-64-2004-shippable-qr fission 1,727,601.33 -> 1,714,876.00
1% Base Content JS macosx1015-64-shippable-qr fission 1,722,269.33 -> 1,709,890.67
1% Base Content JS macosx1015-64-shippable-qr fission 1,722,090.67 -> 1,709,872.00

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=31486

You need to log in before you can comment on or make changes to this bug.