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.
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
Created attachment 363057 [details] WIP - Patch
Created attachment 363060 [details] WIP - Patch
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
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
Created attachment 363233 [details] Patch
Created attachment 365027 [details] Patch
Ping Review
Created attachment 366398 [details] Patch
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.
Created attachment 366709 [details] Patch
Comment on attachment 366709 [details] Patch Thank you very much for the review!
Comment on attachment 366709 [details] Patch Clearing flags on attachment: 366709 Committed r243875: <https://trac.webkit.org/changeset/243875>
All reviewed patches have been landed. Closing bug.
<rdar://problem/49611795>
(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?
(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.