1721413 - Use JSString instead of JSAtom for string literals during instantiation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Tooru Fujisawa [:arai]]

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 (?)

Comment 1

[Tooru Fujisawa [:arai]]

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.

Comment 2

[Tooru Fujisawa [:arai]]

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

Comment 3

[Tooru Fujisawa [:arai]]

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

Comment 4

[Tooru Fujisawa [:arai]]

(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.

Comment 5

[Tooru Fujisawa [:arai]]

(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.

Comment 6

[Tooru Fujisawa [:arai]]

If we use JSExternalString, we need Latin1Char flavor

Comment 7

[Tooru Fujisawa [:arai]]

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

Comment 8

[Tooru Fujisawa [:arai]]

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.

Comment 9

[Tooru Fujisawa [:arai]]

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

Comment 10

[Tooru Fujisawa [:arai]]

current patch stack:
https://treeherder.mozilla.org/jobs?repo=try&revision=104761184bd74004f3e03bc2b34d960d50c274d9&group_state=expanded
https://hg.mozilla.org/try/pushloghtml?changeset=93592931ca97e456b8614834f2db1707aa177839

still hitting zone/tenured heap issue around cloning.

Comment 11

[Tooru Fujisawa [:arai]]

seems to be working
https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=cf8a5d81fcb7e4661e10bc0ca66a5654a1ebc7c3

Comment 12

[Tooru Fujisawa [:arai]]

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

Comment 13

[Tooru Fujisawa [:arai]]

current patch stack, after rebasing on self-hosting patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e211117b4a1a880de4174b3f77633d7cf4e6a10
https://hg.mozilla.org/try/pushloghtml?changeset=4e211117b4a1a880de4174b3f77633d7cf4e6a10

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1721413 - Part 0: Remove unused field from PrivateOpEmitter. r?tcampbell!

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1721413 - Part 1: Collect atomization information for ParserAtom during compilation. r?tcampbell!

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

Depends on D122809

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1721413 - Part 2: Support non-atom string in JSScript gcthings. r?tcampbell!

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

Depends on D122810

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Bug 1721413 - Part 3: Support non-atom string in JSOp::String operand. r?tcampbell!

Depends on D122811

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: 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!

Depends on D122812

Comment 19

[Tooru Fujisawa [:arai]]

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

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Bug 1721413 - Part 5 - Always use JSAtom for short latin1 strings. r?tcampbell!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/78c378e9d9d2
https://hg.mozilla.org/mozilla-central/rev/8bbbcc59e36b
https://hg.mozilla.org/mozilla-central/rev/114dc6e44153
https://hg.mozilla.org/mozilla-central/rev/06561b972cc8
https://hg.mozilla.org/mozilla-central/rev/18f4294ec0a2
https://hg.mozilla.org/mozilla-central/rev/ba3814055b37

Comment 23

[Acasandrei Beatrice (needinfo me)]

== 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