1660275 - Snapshot scopes for parser input, except delazification

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Kannan Vijayan [:djvj]]

Scope information needs to be snapshotted at the start of parse so that we can eliminate the last of the need for in-parser conversion from ParserAtom* to JSAtom* when a variable lookup in the parser reaches up into the active VM scope.

Comment 1

[Tooru Fujisawa [:arai]]

EmitterScope::searchInEnclosingScope needs rewrite.
This is used by delazification and standalone function.

For delazification, we can build a list of free variables while compiling enclosing script, and store them just like closed-over binding,
and need to snapshot them when delazifying.

I'm not yet sure why standalone function needs it instead of using fallback location.
maybe somewhere around https://searchfox.org/mozilla-central/rev/9399e5832979755cd340383f4ca4069dd5fc7774/js/src/vm/CompilationAndEvaluation.cpp#321-343

will look into it.

Comment 2

[Tooru Fujisawa [:arai]]

standalone function's scope handling is for event handler
https://searchfox.org/mozilla-central/rev/9399e5832979755cd340383f4ca4069dd5fc7774/dom/events/EventListenerManager.cpp#1014-1050

I wonder if we should split them (new Function and event handler) into different mode, for clarity

Comment 3

[Tooru Fujisawa [:arai]]

it's empty scope in either way, so we don't have to iterate over scope for standalone function
https://searchfox.org/mozilla-central/rev/9399e5832979755cd340383f4ca4069dd5fc7774/js/src/vm/EnvironmentObject.cpp#875-919

Comment 4

[Tooru Fujisawa [:arai]]

private name also needs snapshot
https://searchfox.org/mozilla-central/rev/9399e5832979755cd340383f4ca4069dd5fc7774/js/src/frontend/Parser.cpp#1508-1549

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 1: Use CompilationAtomCache in ParserAtomsTable::internJSAtom parameter.

Later patch needs to call internJSAtom without CompilationStencil reference.

Depends on D103548

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 2: Directly use usedNames_ field.

Depends on D103575

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 3: Add ScopeContext::init for fallible operation.

Later patch adds fallible operation for cache initialization.

Also split the code for caching CompilationInput.enclosingScope into
ScopeContext::cacheEnclosingScope, with CompilationInput.enclosingScope
instead of CompilationInput.nonDefaultEnclosingScope(), to avoid relying on
the default value for default enclosing scope.

Depends on D103576

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 4: Cache enclosing lexical bindings when compiling eval.

Depends on D103577

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 5: Store free names in lazy functions.

Depends on D103578

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 6: Cache free name locations for delazification.

Depends on D103579

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 6: Cache enclosingScope->environmentChainLength().

Depends on D103580

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 7: Rewrite the comment for CompilationInput.enclosingScope based on CompilationTarget.

Depends on D103581

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 8: Add ScopeContext.hasFunctionNeedsHomeObjectOnChain for eval.

Depends on D103582

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 9: Cache private fields in enclosing scope for eval.

Depends on D103583

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 10: Use CompilationTarget to detect standalone function in non-syntactic scope.

Depends on D103584

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 11: Remove ScopeContext.effectiveScope field and use local variable instead.

Depends on D103585

Comment 17

[Tooru Fujisawa [:arai]]

in raptor tp6 yahoo-mail largest JS file,
GCThingData increases 35%,
and the total XDR size increases by 2%

	before	after	Diff	Diff (%)
Version	22	22	0	0%
AtomTable	1,776,884	1,780,685	+3,801	+0.2%
ImmutableScriptData	1,966,508	1,970,680	+4,172	+0.2%
Scope	150,149	150,610	+461	+0.3%
ScopeName	193,868	194,348	+480	+0.2%
BigInt	1,727	1,732	+5	+0.2%
Object	163,178	163,411	+233	+0.1%
RegExp	2,239	2,244	+5	+0.2%
GCThingData	468,492	631,456	+162,964	+34.7%
ScriptData	626,507	626,864	+357	0%
ScriptExtra	551,488	551,488	0	0%
SharedData	4,696	4,713	+17	+0.3%
Module	0	0	0	-
Source	3,071,613	3,071,613	0	0%
Chunk	23,485	23,570	+85	+0.3%
Total	9,000,856	9,173,436	+172,580	+1.9%

Comment 18

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/perfherder/compare?originalProject=mozilla-central&newProject=try&newRevision=cf48e33ae3bf2ce6e1d6aae68f9ad470ae51a9c3&framework=4&selectedTimeRange=172800

with awsy,

• 2.07% regression on "Base Content JS opt"
• 2.19% regression on "JS opt tp6"

most likely from free names in gcthings

I wonder if we're storing too much.
will look into the details.

Comment 19

[Tooru Fujisawa [:arai]]

the one concern is that, all free names are stored into gcthings, even if it's not declared anywhere in the source,
like, built-in objects (Array, Date, Math, etc), or window property (document, window, console, etc).

at the point of syntax parsing inner function, we don't know if the name will be declared as var in the enclosing scope after the function.
so we cannot just filter out such case, with current way.

we could defer the lazy script's gcthings calculation until the end of parse, and store only "declared-by-enclosing-scope names
(that will become NameLocation::EnvironmentCoordinate) into lazy script gcthings,
but that will require much more refactoring.

maybe we should add special-handling for frequently-used free names.

Comment 20

[Tooru Fujisawa [:arai]]

top 100 free names stored into lazy script gcthings, across all scripts (browser, content, etc)
while: run Firefox, wait for 1 minutes, close.

There are some built-in, and global properties, but not notably too many compared to other cases.

Also, internal dot name is stored into free names list, so the patch is wrong about sort order with offset.
(it uses special offset for internal dot name)

count	name	note
398	Object	built-in
283	Error	built-in
279	C	-
205	undefined	global property
193	a	-
192	n	-
182	t	-
175	ctypes	-
172	r	-
170	e	-
156	TypeError	built-in
145	UnixFile	-
144	Services	-
141	File	-
139	c	-
139	.this	internal dot name
136	o	-
136	i	-
133	Array	built-in
132	.initializers	internal dot name
127	b	-
125	Const	-
117	k	-
109	l	-
99	u	-
95	window	window property
95	document	window property
95	Math	built-in
94	Date	built-in
93	common_Actions_jsm__WEBPACK_IMPORTED_MODULE_0__	-
92	String	built-in
88	G	-
87	Type	-
83	s	-
83	M	-
80	Log	-
79	J	-
78	p	-
77	x	-
75	throw_on_negative	-
75	JSON	built-in
74	d	-
71	S	-
70	E	-
68	H	-
66	Ci	-
63	P	-
62	y	-
62	m	-
59	Z	-
57	Cu	-
56	f	-
56	Promise	built-in
56	OS	-
52	v	-
51	browser	-
51	N	-
50	react__WEBPACK_IMPORTED_MODULE_3___default	-
50	external_React_default	-
50	O	-
50	ChromeUtils	-
49	q	-
49	console	window property
45	h	-
45	I	-
44	Reflect	built-in
40	exports	-
39	global	-
39	g	-
39	U	-
39	T	-
38	setTimeout	window property
37	SharedAll	-
36	encodeURIComponent	built-in
36	action	-
35	z	-
35	Et	-
35	D	-
35	B	-
34	le	-
34	V	-
33	RegExp	built-in
33	A	-
32	tt	-
32	resolve	-
32	W	-
32	Cc	-
31	X	-
30	SysFile	-
30	OSError	-
30	Number	built-in
30	L	-
28	Ht	-
28	F	-
28	Config	-
27	reject	-
27	name	-
27	Function	built-in
26	se	-

Comment 21

[Tooru Fujisawa [:arai]]

also top 100 free names, across all scripts (browser, content, etc)
while running raptor tp1 yahoo-mail, single page load.

here too, built-in appears, but not too much compared to other cases.

count	name	note
5462	Object	built-in
3465	e	-
2349	s	-
1999	r	-
1994	t	-
1984	.this	internal dot name
1817	o	-
1516	a	-
1486	n	-
1273	i	-
1234	c	-
1193	g	-
1169	h	-
937	u	-
926	v	-
761	p	-
759	d	-
749	Error	built-in
724	l	-
699	Fo	-
654	Array	built-in
580	E	-
545	window	window property
533	Promise	built-in
520	m	-
516	f	-
514	Date	built-in
514	.initializers	internal dot name
487	_	-
442	Math	built-in
436	y	-
417	S	-
391	JSON	built-in
373	TypeError	built-in
369	b	-
361	document	window property
350	N	-
344	T	-
342	O	-
320	C	-
304	setTimeout	window property
300	A	-
275	B	-
272	w	-
267	I	-
250	k	-
239	_i	-
238	Bo	-
231	String	built-in
229	P	-
226	D	-
224	undefined	global property
214	F	-
211	x	-
206	R	-
203	Di	-
194	U	-
194	L	-
193	M	-
188	parseInt	built-in
186	common_Actions_jsm__WEBPACK_IMPORTED_MODULE_0__	-
184	Number	built-in
183	Services	-
181	z	-
168	bi	-
163	console	window property
158	K	-
157	Q	-
156	clearTimeout	window property
153	Wo	-
151	V	-
143	Ps	-
141	J	-
140	Ki	-
136	G	-
135	fi	-
134	H	-
131	W	-
128	oe	-
128	RegExp	built-in
126	j	-
125	encodeURIComponent	built-in
125	ee	-
124	q	-
115	Symbol	built-in
111	Cu	-
110	$	-
109	Vo	-
107	zn	-
107	Boolean	built-in
105	ctypes	-
104	Y	-
102	Z	-
100	react__WEBPACK_IMPORTED_MODULE_3___default	-
100	external_React_default	-
99	X	-
97	Jo	-
97	File	-
87	UnixFile	-
87	Ot	-

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Part 5: Move EmitterScope::searchInEnclosingScope to ScopeContext::searchInDelazificationEnclosingScope for future refactoring.

Snapshotting the lazy script free names results in memory footprint regression,
so instead of snapshotting, add an interface for query, without modifying the
underlying impl.

Delazification situation will change once we introduce delazification from
initial stencil, and we can revisit the design then.

Depends on D103578

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/998002007c59
Part 1: Use CompilationAtomCache in ParserAtomsTable::internJSAtom parameter. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/8a776137cb9c
Part 2: Directly use usedNames_ field. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/d81a49d52423
Part 3: Add ScopeContext::init for fallible operation. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/8b70fbd7fa25
Part 4: Cache enclosing lexical bindings when compiling eval. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/88423b2b1272
Part 5: Move EmitterScope::searchInEnclosingScope to ScopeContext::searchInDelazificationEnclosingScope for future refactoring. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/cf61a2d853d3
Part 6: Cache enclosingScope->environmentChainLength(). r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/b433f5a7c637
Part 7: Rewrite the comment for CompilationInput.enclosingScope based on CompilationTarget. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/32e26874a11e
Part 8: Add ScopeContext.hasFunctionNeedsHomeObjectOnChain for eval. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/5e36afd27c5e
Part 9: Cache private fields in enclosing scope for eval. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/c9b03e75026d
Part 10: Use CompilationTarget to detect standalone function in non-syntactic scope. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/cff79f4597a5
Part 11: Remove ScopeContext.effectiveScope field and use local variable instead. r=mgaudet

Comment 24

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/998002007c59
https://hg.mozilla.org/mozilla-central/rev/8a776137cb9c
https://hg.mozilla.org/mozilla-central/rev/d81a49d52423
https://hg.mozilla.org/mozilla-central/rev/8b70fbd7fa25
https://hg.mozilla.org/mozilla-central/rev/88423b2b1272
https://hg.mozilla.org/mozilla-central/rev/cf61a2d853d3
https://hg.mozilla.org/mozilla-central/rev/b433f5a7c637
https://hg.mozilla.org/mozilla-central/rev/32e26874a11e
https://hg.mozilla.org/mozilla-central/rev/5e36afd27c5e
https://hg.mozilla.org/mozilla-central/rev/c9b03e75026d
https://hg.mozilla.org/mozilla-central/rev/cff79f4597a5

Comment 25

[Dorel Luca [:dluca]]

Looks like changes here: https://hg.mozilla.org/mozilla-central/rev/5e36afd27c5e4c7c2090486c60be178cc12c6c1d caused a Bugzilla linting on central: https://treeherder.mozilla.org/jobs?repo=mozilla-central&group_state=expanded&selectedTaskRun=R3UMT8_URj-jwyvo4c14Gw.0&resultStatus=testfailed%2Cbusted%2Cexception&searchStr=linting%2Copt%2Csource-test-file-metadata-bugzilla-components%2Cbugzilla&revision=ca7a3f92939dae5d54226dbc237120bef5d138e9

Arai: Can you please take a look ?

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Bug 1660275 - Remove unnecessary file.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df606e05d6e0
Remove unnecessary file. r=tcampbell

Comment 28

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/df606e05d6e0