Bug 194191 – [JSC] Shrink size of FunctionExecutable

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Shrink size of FunctionExecutable

[Yusuke Suzuki]

Created attachment 360971 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

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

[Yusuke Suzuki]

Created attachment 360984 [details]
Patch

[Yusuke Suzuki]

Created attachment 360986 [details]
Patch

[Yusuke Suzuki]

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.

[Michael Saboff]

Comment on attachment 360986 [details]
Patch

r=me

[Mark Lam]

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.

[Yusuke Suzuki]

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.

[Dominik Inführ]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r240938: <https://trac.webkit.org/changeset/240938>

[Radar WebKit Bug Importer]

<rdar://problem/47796473>