Bug 194944 – [JSC] We should consider moving UnlinkedFunctionExecutable::m_parentScopeTDZVariables to RareData

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

Based on profiling data on Speedometer2, JetStream2, Kranken, Octane and LongSpider, `UnlinkedFunctionExecutable::m_parentScopeTDZVariables` often is carrying an empty VariableEnvironment (see results below).
In such case, we should consider move it to RareData and ensure it only when `parentScopeTDZVariables` is not empty.

[Caio Lima]

Data collected from Speedometer2

Total number of UFE: 39463
Total number of non-empty parentScopeTDZVars: 428

Data collected from JetStream2

Total number of UFE: 83715
Total number of non-empty parentScopeTDZVars:  5285

Data from other benchmarks

Octane/jquery
Total number of UFE: 75334
Total number of non-empty VarEnv: 10

Octane/typescript
Total number of UFE: 1773
Total number of non-empty VarEnv: 10

Octane/pdfjs
Total number of UFE: 1047
Total number of non-empty VarEnv: 10

[Caio Lima]

Created attachment 363057 [details]
WIP - Patch

[Caio Lima]

Created attachment 363060 [details]
WIP - Patch

[EWS Watchlist]

Comment on attachment 363060 [details]
WIP - Patch

Attachment 363060 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11299222

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 363074 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Caio Lima]

Created attachment 363233 [details]
Patch

[Caio Lima]

Created attachment 365027 [details]
Patch

[Caio Lima]

Ping Review

[Caio Lima]

Created attachment 366398 [details]
Patch

[Keith Miller]

Comment on attachment 366398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366398&action=review

r=me with some comments.

> Source/JavaScriptCore/ChangeLog:3
> +        [JSC] We should consider move UnlinkedFunctionExecutable::m_parentScopeTDZVariables to RareData

typo: move=>moving.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1278
> +        bool m_cachedVariablesUnderTDZIsEmpty { false };

Nit: I think this is clearer as m_hasCachedVariablesUnderTDZ since that's the active tense.

[Caio Lima]

Created attachment 366709 [details]
Patch

[Caio Lima]

Comment on attachment 366709 [details]
Patch

Thank you very much for the review!

[WebKit Commit Bot]

Comment on attachment 366709 [details]
Patch

Clearing flags on attachment: 366709

Committed r243875: <https://trac.webkit.org/changeset/243875>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49611795>

[Saam Barati]

(In reply to Caio Lima from comment #1)
> Data collected from Speedometer2
> 
> Total number of UFE: 39463
> Total number of non-empty parentScopeTDZVars: 428
> 
> Data collected from JetStream2
> 
> Total number of UFE: 83715
> Total number of non-empty parentScopeTDZVars:  5285
> 
> Data from other benchmarks
> 
> Octane/jquery
> Total number of UFE: 75334
> Total number of non-empty VarEnv: 10
> 
> Octane/typescript
> Total number of UFE: 1773
> Total number of non-empty VarEnv: 10
> 
> Octane/pdfjs
> Total number of UFE: 1047
> Total number of non-empty VarEnv: 10

What about all of JetStream 2?

[Caio Lima]

(In reply to Saam Barati from comment #16)
> (In reply to Caio Lima from comment #1)
> > Data collected from Speedometer2
> > 
> > Total number of UFE: 39463
> > Total number of non-empty parentScopeTDZVars: 428
> > 
> > Data collected from JetStream2
> > 
> > Total number of UFE: 83715
> > Total number of non-empty parentScopeTDZVars:  5285
> > 
> > Data from other benchmarks
> > 
> > Octane/jquery
> > Total number of UFE: 75334
> > Total number of non-empty VarEnv: 10
> > 
> > Octane/typescript
> > Total number of UFE: 1773
> > Total number of non-empty VarEnv: 10
> > 
> > Octane/pdfjs
> > Total number of UFE: 1047
> > Total number of non-empty VarEnv: 10
> 
> What about all of JetStream 2?

Numbers shows that 6% of total number of UnlinkedFunctionExecutable have non-empty VariableEnvironment. I'm not sure what is the policy to move something to RareData, but I think we should consider this a case to be rare.