1688534 - Optimize object literal instantiation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Tooru Fujisawa [:arai]]

https://searchfox.org/mozilla-central/rev/7431701c98da9218fda314a54efa1a3093760f65/js/src/frontend/ObjLiteral.cpp#59-95

InterpretObjLiteralObj builds key/value array and then calls NewPlainObjectWithProperties.

there:

• we should be able to pre-calculate the number of properties [1]
• we don't have to build an array, if we inline AddPlainObjectProperties there [2]
• js::NativeDefineProperty checks object type each time, but it's always plain object [3]
• we should be able to check if there's duplicate property during compilation, and if there's not, we don't have to lookup the property before add [4]
• there's no accessor. it's always data property [5]

[1] https://searchfox.org/mozilla-central/rev/7431701c98da9218fda314a54efa1a3093760f65/js/src/vm/ObjectGroup.cpp#307
[2] https://searchfox.org/mozilla-central/rev/7431701c98da9218fda314a54efa1a3093760f65/js/src/vm/ObjectGroup.cpp#286-301
[3] https://searchfox.org/mozilla-central/rev/7431701c98da9218fda314a54efa1a3093760f65/js/src/vm/NativeObject.cpp#1535-1592
[4] https://searchfox.org/mozilla-central/rev/7431701c98da9218fda314a54efa1a3093760f65/js/src/vm/NativeObject.cpp#1594-1620
[5] https://searchfox.org/mozilla-central/rev/7431701c98da9218fda314a54efa1a3093760f65/js/src/vm/NativeObject.cpp#1308-1310

Comment 1

[Tooru Fujisawa [:arai]]

While instantiating yahoo-mail's JS after off-thread decoding, 13% of the ScriptDecodeTask::parse (or 17% of frontend::instantiteStencils) is taken by js::InterpretObjLiteral.

Comment 2

[Jan de Mooij [:jandem]]

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

• we don't have to build an array, if we inline AddPlainObjectProperties there [2]

Nice. I think the array was there for TI reasons, to get the right group, but that code has been removed.

• we should be able to check if there's duplicate property during compilation, and if there's not, we don't have to lookup the property before add [4]
• there's no accessor. it's always data property [5]

Maybe you can use NativeObject::addEnumerableDataProperty? That has a fast path for "single child shape already exists in the property tree", I don't know how common that is for object literals but it doesn't seem unlikely for object literals to have a similar structure.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)

Maybe you can use NativeObject::addEnumerableDataProperty? That has a fast path for "single child shape already exists in the property tree", I don't know how common that is for object literals but it doesn't seem unlikely for object literals to have a similar structure.

That is called by AddDataPropertyNonDelegate. Maybe that's a nice one to use because we know object literals aren't delegates (= prototype of another object)...

Comment 4

[Tooru Fujisawa [:arai]]

mildly depends on bug 1687428, for duplicate field part.

(In reply to Jan de Mooij [:jandem] from comment #3)

(In reply to Jan de Mooij [:jandem] from comment #2)

Maybe you can use NativeObject::addEnumerableDataProperty? That has a fast path for "single child shape already exists in the property tree", I don't know how common that is for object literals but it doesn't seem unlikely for object literals to have a similar structure.

That is called by AddDataPropertyNonDelegate. Maybe that's a nice one to use because we know object literals aren't delegates (= prototype of another object)...

Thanks!

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1688534 - Part 1: Store the number of properties/elements in ObjLiteralStencil.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Bug 1688534 - Part 2: Inline NewPlainObjectWithProperties to avoid building properties vector.

Depends on D103550

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Bug 1688534 - Part 3: Add HasIndexOrDuplicatePropName flags.

These flags are used in the next patch.

Depends on D103551

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Bug 1688534 - Part 4: Add fast path for object literal without index or duplicate properties.

Depends on D103552

Comment 9

[Tooru Fujisawa [:arai]]

on raptor tp6 yahoo-mail, 3% improvement (most frequent score doesn't change, but the frequency of bad score reduces).
also, 99% of the object literal hits the optimized case (no index property, no duplicate properties).

Comment 10

[Tooru Fujisawa [:arai]]

tested using BloomFilter instead of HashSet, to detect property duplication.

storage	optimized case	missed optimizable case
HashSet	98.99%	-
BloomFilter<12>	98.97%	0.02%
BloomFilter<11>	98.93%	0.06%
BloomFilter<10>	98.86%	0.13%
BloomFilter<9>	98.72%	0.27%
BloomFilter<8>	98.34%	0.66%
BloomFilter<7>	97.43%	1.58%
BloomFilter<6>	96.06%	2.96%

also I don't see notable difference in the parse mark result, for any case (with HashMap, with BloomFilter, without duplication detection)

Comment 11

[Tooru Fujisawa [:arai]]

reverted to use HashSet, and filed bug 1690274 for bloom filter.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c82c4f218ee
Part 1: Store the number of properties/elements in ObjLiteralStencil. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/ae4890a14011
Part 2: Inline NewPlainObjectWithProperties to avoid building properties vector. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/50f5c8ec9472
Part 3: Add HasIndexOrDuplicatePropName flags. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/2ce5fee4f1b3
Part 4: Add fast path for object literal without index or duplicate properties. r=tcampbell

Comment 13

[Alexandru Michis [:malexandru]]

Backed out 4 changesets (Bug 1688534) for causing hazard bustages in ObjLiteral.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/a89a0ec9271b13d004254f0ff474c168a44186e9

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=linux%2Cx64%2Cdebug%2Cspidermonkey%2Cbuilds%2Chazard-linux64-shell-haz%2Fdebug%2Ch&tochange=a89a0ec9271b13d004254f0ff474c168a44186e9&fromchange=37c94de87a83f91a2d79f9e0b93065984022623e

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328595713&repo=autoland&lineNumber=5441

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7acd2b3e2e5a
Part 1: Store the number of properties/elements in ObjLiteralStencil. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/118670433729
Part 2: Inline NewPlainObjectWithProperties to avoid building properties vector. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/9e40a25189e8
Part 3: Add HasIndexOrDuplicatePropName flags. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/0e1735c489aa
Part 4: Add fast path for object literal without index or duplicate properties. r=tcampbell

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7acd2b3e2e5a
https://hg.mozilla.org/mozilla-central/rev/118670433729
https://hg.mozilla.org/mozilla-central/rev/9e40a25189e8
https://hg.mozilla.org/mozilla-central/rev/0e1735c489aa