Bug 190340 – [JSC] JSC should have "parseFunction" to optimize Function constructor

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

In Function constructor, we parse code three times if we have `Function("p0", "p1", "body")` since we need to ensure

1. parameters are syntax valid
2. body is syntax valid
3. parse all the things together

We can optimize this situation by having a special customized function parseFunction, which will take the end position of the parameters.

[Yusuke Suzuki]

web-tooling-benchmark/uglify-js heavily stresses Function constructor.

[Yusuke Suzuki]

Created attachment 351744 [details]
Patch

WIP

[Yusuke Suzuki]

OK, ready.

[Yusuke Suzuki]

Created attachment 351746 [details]
Patch

[EWS Watchlist]

Comment on attachment 351746 [details]
Patch

Attachment 351746 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9484297

New failing tests:
fast/events/attribute-listener-deletion-crash.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/inline-event-handler-ordering.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute.html
fast/dom/attribute-event-listener-errors.html
fast/events/window-onerror-syntax-error-in-attr.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror.html
js/dom/invalid-syntax-for-function.html
js/dom/script-start-end-locations.html

[EWS Watchlist]

Created attachment 351751 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351746 [details]
Patch

Attachment 351746 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9484432

New failing tests:
fast/events/attribute-listener-deletion-crash.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late.html
js/dom/script-start-end-locations.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/inline-event-handler-ordering.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute.html
fast/dom/attribute-event-listener-errors.html
fast/events/window-onerror-syntax-error-in-attr.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror.html
js/dom/invalid-syntax-for-function.html

[EWS Watchlist]

Created attachment 351754 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351746 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/inline-event-handler-ordering.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute.html
fast/dom/attribute-event-listener-errors.html
fast/events/window-onerror-syntax-error-in-attr.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror.html
js/dom/invalid-syntax-for-function.html
js/dom/script-start-end-locations.html

[EWS Watchlist]

Created attachment 351756 [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 351746 [details]
Patch

Attachment 351746 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9485386

New failing tests:
fast/events/attribute-listener-deletion-crash.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/inline-event-handler-ordering.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute.html
fast/dom/attribute-event-listener-errors.html
fast/events/window-onerror-syntax-error-in-attr.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror.html
js/dom/invalid-syntax-for-function.html
js/dom/script-start-end-locations.html

[EWS Watchlist]

Created attachment 351757 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351746 [details]
Patch

Attachment 351746 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9486068

New failing tests:
fast/events/attribute-listener-deletion-crash.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/inline-event-handler-ordering.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute.html
fast/dom/attribute-event-listener-errors.html
fast/events/window-onerror-syntax-error-in-attr.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror.html
js/dom/invalid-syntax-for-function.html
js/dom/script-start-end-locations.html

[EWS Watchlist]

Created attachment 351760 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351746 [details]
Patch

Attachment 351746 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9486115

New failing tests:
js/dom/invalid-syntax-for-function.html
fast/events/attribute-listener-deletion-crash.html
js/dom/script-start-end-locations.html
fast/dom/attribute-event-listener-errors.html
fast/events/window-onerror-syntax-error-in-attr.html

[EWS Watchlist]

Created attachment 351761 [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 351746 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/events/invalid-uncompiled-raw-handler-compiled-late.html
imported/w3c/web-platform-tests/html/webappapis/scripting/events/inline-event-handler-ordering.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-attribute.html
fast/dom/attribute-event-listener-errors.html
fast/events/window-onerror-syntax-error-in-attr.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/compile-error-in-body-onerror.html
js/dom/invalid-syntax-for-function.html
js/dom/script-start-end-locations.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 351746 [details]
Patch

Attachment 351746 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9487240

New failing tests:
js/dom/invalid-syntax-for-function.html
fast/events/attribute-listener-deletion-crash.html
js/dom/script-start-end-locations.html
fast/dom/attribute-event-listener-errors.html
fast/events/window-onerror-syntax-error-in-attr.html

[EWS Watchlist]

Created attachment 351767 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Yusuke Suzuki]

Created attachment 351769 [details]
Patch

[Yusuke Suzuki]

Created attachment 351770 [details]
Patch

[Yusuke Suzuki]

Created attachment 351774 [details]
Patch

[Caio Lima]

Comment on attachment 351774 [details]
Patch

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

Nice patch! Giving some comments in the code.

> Source/JavaScriptCore/parser/Parser.cpp:2502
> +            semanticFailIfFalse(lastTokenEndPosition().offset == *parametersEndPosition, "Parameters should be composed of arguments offered for parameters in Function constructor");

I'm not sure if I'm right here because of some edge cases, but maybe the message "Parameters should match arguments offered as parameters in Function constructor" is more clear about the error.

> Source/JavaScriptCore/parser/Parser.cpp:2935
> +            failIfFalse((parseFunctionInfo(context, FunctionNameRequirements::Unnamed, parseMode, false, isConstructor ? constructorKind : ConstructorKind::None, SuperBinding::Needed, methodStart, methodInfo, FunctionDefinitionType::Method, std::nullopt)), "Cannot parse this method");

Maybe making ```parametersEndPosition``` default to ```std::nullopt``` in ```parseFunctionInfo``` would make this code more readable.

> Source/JavaScriptCore/parser/Parser.cpp:4078
> +    failIfFalse((parseFunctionInfo(context, FunctionNameRequirements::Unnamed, parseMode, false, ConstructorKind::None, SuperBinding::Needed, methodStart, methodInfo, FunctionDefinitionType::Method, std::nullopt)), "Cannot parse this method");

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:4113
> +        failIfFalse((parseFunctionInfo(context, FunctionNameRequirements::Unnamed, SourceParseMode::GetterMode, false, constructorKind, SuperBinding::Needed, getterOrSetterStartOffset, info, FunctionDefinitionType::Method, std::nullopt)), "Cannot parse getter definition");

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:4116
> +        failIfFalse((parseFunctionInfo(context, FunctionNameRequirements::Unnamed, SourceParseMode::SetterMode, false, constructorKind, SuperBinding::Needed, getterOrSetterStartOffset, info, FunctionDefinitionType::Method, std::nullopt)), "Cannot parse setter definition");

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:4368
> +    failIfFalse((parseFunctionInfo(context, FunctionNameRequirements::None, parseMode, false, ConstructorKind::None, SuperBinding::NotNeeded, functionKeywordStart, functionInfo, FunctionDefinitionType::Expression, std::nullopt)), "Cannot parse function expression");

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:4386
> +    failIfFalse(parseFunctionInfo(context, FunctionNameRequirements::None, parseMode, false, ConstructorKind::None, SuperBinding::NotNeeded, functionKeywordStart, functionInfo, FunctionDefinitionType::Expression, std::nullopt), parseMode == SourceParseMode::AsyncFunctionMode ? "Cannot parse async function expression" : "Cannot parse async generator function expression");

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:4891
> +    failIfFalse((parseFunctionInfo(context, FunctionNameRequirements::Unnamed, parseMode, true, ConstructorKind::None, SuperBinding::NotNeeded, functionKeywordStart, info, FunctionDefinitionType::Expression, std::nullopt)), "Cannot parse arrow function expression");

Ditto.

[Mark Lam]

Comment on attachment 351774 [details]
Patch

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

r=me

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:177
> +    JSObject*& exception, int overrideLineNumber, std::optional<int> parametersEndPosition)

Since parametersEndPosition is only ever used for FunctionConstructors, can you rename it as functionConstructorParametersEndPosition everywhere below?  I think that that documents its intent better.

> Source/JavaScriptCore/parser/Parser.cpp:247
> +        else {
> +            if (parsingContext == ParsingContext::FunctionConstructor)
> +                sourceElements = parseSingleFunction(context, parametersEndPosition);
> +            else
> +                sourceElements = parseSourceElements(context, CheckForStrictMode);
> +        }

No need to indent this.  You can just express this as:

    else if (parsingContext == ParsingContext::FunctionConstructor)
        sourceElements = parseSingleFunction(context, parametersEndPosition);
    else
        sourceElements = parseSourceElements(context, CheckForStrictMode);

>> Source/JavaScriptCore/parser/Parser.cpp:2502
>> +            semanticFailIfFalse(lastTokenEndPosition().offset == *parametersEndPosition, "Parameters should be composed of arguments offered for parameters in Function constructor");
> 
> I'm not sure if I'm right here because of some edge cases, but maybe the message "Parameters should match arguments offered as parameters in Function constructor" is more clear about the error.

I like Caio's suggestion "Parameters should match arguments offered as parameters in Function constructor" because it's more concise.

>> Source/JavaScriptCore/parser/Parser.cpp:2935
>> +            failIfFalse((parseFunctionInfo(context, FunctionNameRequirements::Unnamed, parseMode, false, isConstructor ? constructorKind : ConstructorKind::None, SuperBinding::Needed, methodStart, methodInfo, FunctionDefinitionType::Method, std::nullopt)), "Cannot parse this method");
> 
> Maybe making ```parametersEndPosition``` default to ```std::nullopt``` in ```parseFunctionInfo``` would make this code more readable.

I can go either way with this.  There are already places in the code where you're relying on having a default value for the parametersEndPosition.  For that reason, I'm slightly more include to prefer using a default value for this case as well.  I'll leave this to your discretion.

[Yusuke Suzuki]

Comment on attachment 351774 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:177
>> +    JSObject*& exception, int overrideLineNumber, std::optional<int> parametersEndPosition)
> 
> Since parametersEndPosition is only ever used for FunctionConstructors, can you rename it as functionConstructorParametersEndPosition everywhere below?  I think that that documents its intent better.

Changed.

>> Source/JavaScriptCore/parser/Parser.cpp:247
>> +        }
> 
> No need to indent this.  You can just express this as:
> 
>     else if (parsingContext == ParsingContext::FunctionConstructor)
>         sourceElements = parseSingleFunction(context, parametersEndPosition);
>     else
>         sourceElements = parseSourceElements(context, CheckForStrictMode);

Fixed.

>>> Source/JavaScriptCore/parser/Parser.cpp:2502
>>> +            semanticFailIfFalse(lastTokenEndPosition().offset == *parametersEndPosition, "Parameters should be composed of arguments offered for parameters in Function constructor");
>> 
>> I'm not sure if I'm right here because of some edge cases, but maybe the message "Parameters should match arguments offered as parameters in Function constructor" is more clear about the error.
> 
> I like Caio's suggestion "Parameters should match arguments offered as parameters in Function constructor" because it's more concise.

OK, changed the message.

>>> Source/JavaScriptCore/parser/Parser.cpp:2935
>>> +            failIfFalse((parseFunctionInfo(context, FunctionNameRequirements::Unnamed, parseMode, false, isConstructor ? constructorKind : ConstructorKind::None, SuperBinding::Needed, methodStart, methodInfo, FunctionDefinitionType::Method, std::nullopt)), "Cannot parse this method");
>> 
>> Maybe making ```parametersEndPosition``` default to ```std::nullopt``` in ```parseFunctionInfo``` would make this code more readable.
> 
> I can go either way with this.  There are already places in the code where you're relying on having a default value for the parametersEndPosition.  For that reason, I'm slightly more include to prefer using a default value for this case as well.  I'll leave this to your discretion.

Changed to use std::nullopt default value.

[Yusuke Suzuki]

Committed r237054: <https://trac.webkit.org/changeset/237054>

[Radar WebKit Bug Importer]

<rdar://problem/45211266>

[Saam Barati]

This is a 6% regression on JetStream 2 on iOS

[WebKit Commit Bot]

Re-opened since this is blocked by bug 190593

[Saam Barati]

ARES6 is also regressed by this.

JetStream 2 regressions:
- Air 50% worse.
- Babylong 10% worse
- async-fs 13% worse
- acorn-wtb 20% worse
- cypto-sha1-sp 37% worse
- delta-blue 13% worse

[Yusuke Suzuki]

(In reply to Saam Barati from comment #31)
> ARES6 is also regressed by this.
> 
> JetStream 2 regressions:
> - Air 50% worse.
> - Babylong 10% worse
> - async-fs 13% worse
> - acorn-wtb 20% worse
> - cypto-sha1-sp 37% worse
> - delta-blue 13% worse

Is it reproducible on non-iOS environments? I've just managed to run ARES6/Air on my MacBook Pro, and it seems perf-neutral.

Before this patch.
Running... Air ( 1  to go)
firstIteration:     56.18 +- 17.03 ms
averageWorstCase:   22.78 +- 1.37 ms
steadyState:        5.87 +- 0.10 ms
summary:            22.96 +- 1.69 ms


Running... Basic ( 1  to go)
firstIteration:     26.95 +- 3.97 ms
averageWorstCase:   13.54 +- 2.63 ms
steadyState:        6.88 +- 0.83 ms
summary:            22.96 +- 1.69 ms


Running... Babylon ( 1  to go)
firstIteration:     35.80 +- 5.07 ms
averageWorstCase:   17.42 +- 3.28 ms
steadyState:        5.67 +- 0.08 ms
summary:            22.96 +- 1.69 ms


Running... ML ( 1  to go)
firstIteration:     95.11 +- 4.23 ms
averageWorstCase:   61.22 +- 3.27 ms
steadyState:        56.85 +- 2.85 ms
summary:            22.89 +- 1.29 ms


After this patch.
Running... Air ( 1  to go)
firstIteration:     55.42 +- 14.54 ms
averageWorstCase:   22.96 +- 1.14 ms
steadyState:        5.88 +- 0.06 ms
summary:            22.98 +- 1.27 ms


Running... Basic ( 1  to go)
firstIteration:     26.07 +- 3.85 ms
averageWorstCase:   12.57 +- 1.10 ms
steadyState:        6.64 +- 0.37 ms
summary:            22.98 +- 1.27 ms


Running... Babylon ( 1  to go)
firstIteration:     36.04 +- 5.69 ms
averageWorstCase:   18.40 +- 1.15 ms
steadyState:        5.80 +- 0.14 ms
summary:            22.98 +- 1.27 ms


Running... ML ( 1  to go)
firstIteration:     95.47 +- 4.10 ms
averageWorstCase:   62.02 +- 1.80 ms
steadyState:        57.82 +- 2.64 ms
summary:            22.89 +- 0.99 ms

[Saam Barati]

Our Mac bots do not show a regression.

Unfortunately, our iOS bots went down after your patch landed for ~3 days. I'll need to do some manual testing. Unfortunately, I have some other urgent work I'm doing today. I'll need to help you and run some A/B tests locally, hopefully I'll have time tomorrow.

[Saam Barati]

I just tried to reproduce this locally and could not. Maybe our bots were in a weird state or it was some other revision. Let's reland this and I'll monitor our bots.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #34)
> I just tried to reproduce this locally and could not. Maybe our bots were in
> a weird state or it was some other revision. Let's reland this and I'll
> monitor our bots.

Nice! I'll land it :). Let me know if the regression happens!

[Yusuke Suzuki]

Committed r237254: <https://trac.webkit.org/changeset/237254>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 190760

[Yusuke Suzuki]

Comment on attachment 351774 [details]
Patch

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

> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:129
> +        auto body = args.at(args.size() - 1).toString(exec)->viewWithUnderlyingString(exec);

Not sure why this patch causes regression in iOS devices, but changing this to ` auto body = args.at(args.size() - 1).toWTFString(exec);` could be one attempt.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #38)
> Comment on attachment 351774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351774&action=review
> 
> > Source/JavaScriptCore/runtime/FunctionConstructor.cpp:129
> > +        auto body = args.at(args.size() - 1).toString(exec)->viewWithUnderlyingString(exec);
> 
> Not sure why this patch causes regression in iOS devices, but changing this
> to ` auto body = args.at(args.size() - 1).toWTFString(exec);` could be one
> attempt.

If you send me a patch (or multiple patches to test) I can kick off A/B tasks.

[Saam Barati]

(In reply to Saam Barati from comment #39)
> (In reply to Yusuke Suzuki from comment #38)
> > Comment on attachment 351774 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=351774&action=review
> > 
> > > Source/JavaScriptCore/runtime/FunctionConstructor.cpp:129
> > > +        auto body = args.at(args.size() - 1).toString(exec)->viewWithUnderlyingString(exec);
> > 
> > Not sure why this patch causes regression in iOS devices, but changing this
> > to ` auto body = args.at(args.size() - 1).toWTFString(exec);` could be one
> > attempt.
> 
> If you send me a patch (or multiple patches to test) I can kick off A/B
> tasks.

The patches should be in svn patch format

[Yusuke Suzuki]

(In reply to Saam Barati from comment #40)
> (In reply to Saam Barati from comment #39)
> > (In reply to Yusuke Suzuki from comment #38)
> > > Comment on attachment 351774 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=351774&action=review
> > > 
> > > > Source/JavaScriptCore/runtime/FunctionConstructor.cpp:129
> > > > +        auto body = args.at(args.size() - 1).toString(exec)->viewWithUnderlyingString(exec);
> > > 
> > > Not sure why this patch causes regression in iOS devices, but changing this
> > > to ` auto body = args.at(args.size() - 1).toWTFString(exec);` could be one
> > > attempt.
> > 
> > If you send me a patch (or multiple patches to test) I can kick off A/B
> > tasks.
> 
> The patches should be in svn patch format

I guess the issue is not directly related to this patch since ARES-6/Air is not heavily using Function constructor (correct?). My guess is that Parser.cpp change involves inline/noinline decision in the compiler and causes the regression.

To ensure this, I would like to land the Parser.cpp change separately. In this first attempt, we land the Parser.cpp changes, but we do not use it at all. If it does not cause the regression, it is fine. We should investigate the other issues. But if it causes the regression, we need to add tricky ALWAYS_INLINE / NEVER_INLINE carefully on the actual machine.

[Yusuke Suzuki]

Committed r237445: <https://trac.webkit.org/changeset/237445>

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #42)
> Committed r237445: <https://trac.webkit.org/changeset/237445>

Attempt to roll in the partial change of the patch. It includes Parser.cpp etc. changes. But FunctionConstructor does not use it at all.
If it causes the regression, I think inlining decision of Parser.cpp functions seems culprit. If it does not pose the regression, we can investigate FunctionConstructor's change (it is small).

[Saam Barati]

(In reply to Yusuke Suzuki from comment #43)
> (In reply to Yusuke Suzuki from comment #42)
> > Committed r237445: <https://trac.webkit.org/changeset/237445>
> 
> Attempt to roll in the partial change of the patch. It includes Parser.cpp
> etc. changes. But FunctionConstructor does not use it at all.
> If it causes the regression, I think inlining decision of Parser.cpp
> functions seems culprit. If it does not pose the regression, we can
> investigate FunctionConstructor's change (it is small).

This caused a regression

[WebKit Commit Bot]

Re-opened since this is blocked by bug 190972

[Yusuke Suzuki]

(In reply to Saam Barati from comment #44)
> (In reply to Yusuke Suzuki from comment #43)
> > (In reply to Yusuke Suzuki from comment #42)
> > > Committed r237445: <https://trac.webkit.org/changeset/237445>
> > 
> > Attempt to roll in the partial change of the patch. It includes Parser.cpp
> > etc. changes. But FunctionConstructor does not use it at all.
> > If it causes the regression, I think inlining decision of Parser.cpp
> > functions seems culprit. If it does not pose the regression, we can
> > investigate FunctionConstructor's change (it is small).
> 
> This caused a regression

So, it seems that this regression is due to Parser.cpp's inlining decision change due to the slight code changes, since the added path is not used in the FunctionConstructor.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #46)
> (In reply to Saam Barati from comment #44)
> > (In reply to Yusuke Suzuki from comment #43)
> > > (In reply to Yusuke Suzuki from comment #42)
> > > > Committed r237445: <https://trac.webkit.org/changeset/237445>
> > > 
> > > Attempt to roll in the partial change of the patch. It includes Parser.cpp
> > > etc. changes. But FunctionConstructor does not use it at all.
> > > If it causes the regression, I think inlining decision of Parser.cpp
> > > functions seems culprit. If it does not pose the regression, we can
> > > investigate FunctionConstructor's change (it is small).
> > 
> > This caused a regression
> 
> So, it seems that this regression is due to Parser.cpp's inlining decision
> change due to the slight code changes, since the added path is not used in
> the FunctionConstructor.

First I thought that this patch broke the CodeCache thing, but I investigated and it seems working correctly.
So, Parser.cpp's inlining decision seems culprit.

Is there any way to explore the appropriate `ALWAYS_INLINE` / `NEVER_INLINE` configuration?

[Yusuke Suzuki]

Committed r237492: <https://trac.webkit.org/changeset/237492>

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #48)
> Committed r237492: <https://trac.webkit.org/changeset/237492>

Landed with -Parser.{h,cpp} changes. I believe this does not cause any regression.
And it also ensures that Parser.{h,cpp}'s inlining decision change is the cause of the regression on iOS devices.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #49)
> (In reply to Yusuke Suzuki from comment #48)
> > Committed r237492: <https://trac.webkit.org/changeset/237492>
> 
> Landed with -Parser.{h,cpp} changes. I believe this does not cause any
> regression.
> And it also ensures that Parser.{h,cpp}'s inlining decision change is the
> cause of the regression on iOS devices.

This causes the regression.

[Saam Barati]

(In reply to Saam Barati from comment #50)
> (In reply to Yusuke Suzuki from comment #49)
> > (In reply to Yusuke Suzuki from comment #48)
> > > Committed r237492: <https://trac.webkit.org/changeset/237492>
> > 
> > Landed with -Parser.{h,cpp} changes. I believe this does not cause any
> > regression.
> > And it also ensures that Parser.{h,cpp}'s inlining decision change is the
> > cause of the regression on iOS devices.
> 
> This causes the regression.

I'm going to roll it out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 191035

[Yusuke Suzuki]

(In reply to WebKit Commit Bot from comment #52)
> Re-opened since this is blocked by bug 191035

Interesting! Hm, computeHash would be culprit. I'll land the Parser part which does not include the SourceCode caching part.

[Yusuke Suzuki]

Committed r237586: <https://trac.webkit.org/changeset/237586>

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #54)
> Committed r237586: <https://trac.webkit.org/changeset/237586>

Then, I've just landed the Parser changes, I hope it does not cause any regression.

[Yusuke Suzuki]

Committed r238026: <https://trac.webkit.org/changeset/238026>

[Saam Barati]

Rolling code cache changes out in:
https://bugs.webkit.org/show_bug.cgi?id=191555

[Yusuke Suzuki]

Committed r238185: <https://trac.webkit.org/changeset/238185>

[Yusuke Suzuki]

Committed r238365: <https://trac.webkit.org/changeset/238365>

[Yusuke Suzuki]

https://perf.webkit.org/v3/#/charts?paneList=((19-1))&since=1542035849013
Perf progression in DOM/Event since event handlers use function constructor implementation to materialize a function object.