| Summary: | [JSC] JSC should have "parseFunction" to optimize Function constructor | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, saam, ticaiolima, webkit-bug-importer | ||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||
| Bug Depends on: | 190593, 190760, 190972, 191035 | ||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
web-tooling-benchmark/uglify-js heavily stresses Function constructor. Created attachment 351744 [details]
Patch
WIP
OK, ready. Created attachment 351746 [details]
Patch
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 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
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 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
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 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
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 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
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 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
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 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
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 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
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 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
Created attachment 351769 [details]
Patch
Created attachment 351770 [details]
Patch
Created attachment 351774 [details]
Patch
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. 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. 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. Committed r237054: <https://trac.webkit.org/changeset/237054> This is a 6% regression on JetStream 2 on iOS Re-opened since this is blocked by bug 190593 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 (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 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. 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. (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! Committed r237254: <https://trac.webkit.org/changeset/237254> Re-opened since this is blocked by bug 190760 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. (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. (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 (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. Committed r237445: <https://trac.webkit.org/changeset/237445> (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). (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 Re-opened since this is blocked by bug 190972 (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. (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? Committed r237492: <https://trac.webkit.org/changeset/237492> (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. (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. (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. Re-opened since this is blocked by bug 191035 (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. Committed r237586: <https://trac.webkit.org/changeset/237586> (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. Committed r238026: <https://trac.webkit.org/changeset/238026> Rolling code cache changes out in: https://bugs.webkit.org/show_bug.cgi?id=191555 Committed r238185: <https://trac.webkit.org/changeset/238185> Committed r238365: <https://trac.webkit.org/changeset/238365> 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. |
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.