[JSC] Shrink size of FunctionExecutable
Created attachment 360971 [details] Patch
Comment on attachment 360971 [details] Patch Attachment 360971 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11006433 New failing tests: fast/events/event-function-toString.html js/dom/function-names.html
Created attachment 360972 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 360971 [details] Patch Attachment 360971 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11006452 New failing tests: fast/events/event-function-toString.html js/dom/function-names.html
Created attachment 360973 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 360971 [details] Patch Attachment 360971 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11006478 New failing tests: fast/events/event-function-toString.html js/dom/function-names.html
Created attachment 360974 [details] Archive of layout-test-results from ews113 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 360971 [details] Patch Attachment 360971 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11006541 New failing tests: fast/events/event-function-toString.html js/dom/function-names.html
Created attachment 360975 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 360971 [details] Patch Attachment 360971 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11006462 New failing tests: fast/events/event-function-toString.html js/dom/function-names.html
Created attachment 360976 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 360971 [details] Patch Attachment 360971 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/11006520 New failing tests: typeProfiler.yaml/typeProfiler/return.js.ftl-type-profiler-ftl-eager typeProfiler.yaml/typeProfiler/return.js.ftl-no-cjit-type-profiler-force-poly-proto typeProfiler.yaml/typeProfiler/arrow-functions.js.ftl-type-profiler typeProfiler.yaml/typeProfiler/arrow-functions.js.ftl-type-profiler-ftl-eager typeProfiler.yaml/typeProfiler/return.js.ftl-type-profiler typeProfiler.yaml/typeProfiler/arrow-functions.js.ftl-no-cjit-type-profiler-force-poly-proto apiTests
Comment on attachment 360971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360971&action=review r- because EWS is unhappy > Source/JavaScriptCore/ChangeLog:16 > + the size of FunctionExecutable in the major case. major => common
Created attachment 360984 [details] Patch
Created attachment 360986 [details] Patch
Comment on attachment 360971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360971&action=review Updated, and I think this is OK now. >> Source/JavaScriptCore/ChangeLog:16 >> + the size of FunctionExecutable in the major case. > > major => common Thanks, fixed. > Source/JavaScriptCore/runtime/FunctionExecutable.cpp:114 > + m_rareData = std::make_unique<RareData>(); We should update the members of m_rareData here by using m_unlinkedExecutable. Fixed in the newer patch.
Comment on attachment 360986 [details] Patch r=me
Comment on attachment 360986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360986&action=review > Source/JavaScriptCore/runtime/FunctionExecutable.h:188 > + void setOverrideLineNumber(int overrideLineNumber) > + { > + if (overrideLineNumber == -1) > + return; > + ensureRareData().m_overrideLineNumber = overrideLineNumber; > + } > + > + Optional<int> overrideLineNumber() const > + { > + if (UNLIKELY(m_rareData)) > + return m_rareData->m_overrideLineNumber; > + return WTF::nullopt; > + } If it's possible to setOverrideLineNumber(-1) after setOverrideLineNumber(5), then we need to reflect that in ensureRareData().m_overrideLineNumber i.e. setOverrideLineNumber() should check if m_rareData exists and behave accordingly. Ditto for overrideLineNumber() which needs to return a nullopt if m_rareData exists and m_overrideLineNumber is -1. If this is not true, then you should ASSERT(!m_rareData) in the if (overrideLineNumber == -1) case in setOverrideLineNumber(). OK, by grepping the code, I see that setOverrideLineNumber() is only ever called once. Let's add the ASSERT so that we don't fail silently if things change.
Comment on attachment 360986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360986&action=review >> Source/JavaScriptCore/runtime/FunctionExecutable.h:188 >> + } > > If it's possible to setOverrideLineNumber(-1) after setOverrideLineNumber(5), then we need to reflect that in ensureRareData().m_overrideLineNumber i.e. setOverrideLineNumber() should check if m_rareData exists and behave accordingly. Ditto for overrideLineNumber() which needs to return a nullopt if m_rareData exists and m_overrideLineNumber is -1. > > If this is not true, then you should ASSERT(!m_rareData) in the if (overrideLineNumber == -1) case in setOverrideLineNumber(). > > OK, by grepping the code, I see that setOverrideLineNumber() is only ever called once. Let's add the ASSERT so that we don't fail silently if things change. Nice! Added ASSERT.
I've tested that failing test on ARM (jsc-armv7) locally, the test fails because there is not enough executable memory but succeeds with more.
Comment on attachment 360986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360986&action=review >>> Source/JavaScriptCore/runtime/FunctionExecutable.h:188 >>> + } >> >> If it's possible to setOverrideLineNumber(-1) after setOverrideLineNumber(5), then we need to reflect that in ensureRareData().m_overrideLineNumber i.e. setOverrideLineNumber() should check if m_rareData exists and behave accordingly. Ditto for overrideLineNumber() which needs to return a nullopt if m_rareData exists and m_overrideLineNumber is -1. >> >> If this is not true, then you should ASSERT(!m_rareData) in the if (overrideLineNumber == -1) case in setOverrideLineNumber(). >> >> OK, by grepping the code, I see that setOverrideLineNumber() is only ever called once. Let's add the ASSERT so that we don't fail silently if things change. > > Nice! Added ASSERT. Or, we can just put `m_rareData->m_overrideLineNumber = WTF::nullopt` if m_rareData exists. Maybe, it is more simple. "Ditto for overrideLineNumber() which needs to return a nullopt if m_rareData exists and m_overrideLineNumber is -1." In this case, our Markable<int> makes it WTF::nullopt. So it just works.
Committed r240938: <https://trac.webkit.org/changeset/240938>
<rdar://problem/47796473>