[JSC] Eagerly build AST for likely IIFEs#62019
Conversation
ast-hugger
added
the
JavaScriptCore
|
EWS run on previous version of this PR (hash 5c05986) Details |
🛠 ios-apple
webkit-ews-buildbot
added
the
merging-blocked
ast-hugger
removed
the
merging-blocked
ast-hugger
force-pushed
the
eng/JSC-Eagerly-build-AST-for-likely-IIFEs
branch
from
5c05986 to
6579d3c
Compare
|
EWS run on previous version of this PR (hash 6579d3c) Details
|
ast-hugger
force-pushed
the
eng/JSC-Eagerly-build-AST-for-likely-IIFEs
branch
from
6579d3c to
d0b057b
Compare
|
EWS run on previous version of this PR (hash d0b057b) Details
|
ast-hugger
force-pushed
the
eng/JSC-Eagerly-build-AST-for-likely-IIFEs
branch
from
d0b057b to
b8a616b
Compare
|
EWS run on previous version of this PR (hash b8a616b) Details
|
webkit-ews-buildbot
added
the
merging-blocked
ast-hugger
removed
the
merging-blocked
ast-hugger
force-pushed
the
eng/JSC-Eagerly-build-AST-for-likely-IIFEs
branch
from
b8a616b to
3c8578f
Compare
|
EWS run on previous version of this PR (hash 3c8578f) Details
|
| } | ||
|
|
||
| // Cache line 0 (hot) | ||
| ALWAYS_INLINE void setCurrentArena(ParserArena& arena) { |
There was a problem hiding this comment.
Align to WebKit coding style ({ should be in the next line).
There was a problem hiding this comment.
Interesting why the style checker didn't complain about this one.
| { } | ||
|
|
||
| private: | ||
| ParserArena m_arena; |
There was a problem hiding this comment.
Can we have multiple ParserArena in one parsing implementation? Currently Identifier* is tied to ParserArena's IdentifierArena's allocated Identifier, which means that this pointer will be invalidated when arena gets destroyed.
There was a problem hiding this comment.
Right, but with this change identifier creation is funneled into whatever is m_currentArena->identifierArena(). setCurrentArena() also reassigns the lexer's identifier arena to point at it. So an IIFE is parsed using a temporary arena, and then its FunctionNode takes ownership of that arena's contents and the parser goes back to its own m_arena as m_currentArena.
| // If this is a nested function inside an eagerly parsed IIFE, adjust the | ||
| // start column. During eager parsing columns are absolute, but column counting | ||
| // logic expects them relative to the containing function's '('. | ||
| if (m_iifeParseState | ||
| && !m_iifeParseState->isAvailable() | ||
| && tokenLineStart() <= m_iifeParseState->startOffset()) [[unlikely]] { | ||
|
|
||
| int adjustment = m_iifeParseState->startOffset() - tokenLineStart(); | ||
| startColumn -= adjustment; | ||
| functionInfo.parametersStartColumn = startColumn; | ||
| } |
There was a problem hiding this comment.
Can you describe a bit detail about this with the example code?
I feel like needing a hack here sounds weird and error-prone.
There was a problem hiding this comment.
In normal lazy parsing when a function is nested, columns on its first line are tracked relative to its parameter list start because that's where the SourceCode seen by the parser begins. UnlinkedFunctionExecutable expects that column on the first line are relative. When eagerly parsing, the parser sees the broader uses the original SourceCode that contains the function expression, and the columns are absolute.
This was uncovered by a layout test js/dom/line-column-numbers-expected.html. The test I added with this PR reproduces it. Case 1 is the simplest example. I agree this looks weird. Maybe we should revise column tracking so it's always absolute in the top-level source?
| endLocation.startOffset = functionInfo.endOffset; | ||
| endLocation.lineStartOffset = m_lastTokenLocation.lineStartOffset; | ||
|
|
||
| FunctionNodeUniquePtr functionNode(new FunctionNode( |
There was a problem hiding this comment.
We can't use makeUnique here because FunctionNodeUniquePtr needs a custom deleter, which it needs because SourceProviderCache.h is a project header and it can't include Nodes.h directly, which it would need to have the full definition of FunctionNode. I don't like that custom deleter overall, but not sure which alternative to choose - keep IIFE cache somewhere else, or promote the header, or something else. Suggestions welcome.
There was a problem hiding this comment.
Fixed - IIFE registry is now a separate entity from SourceProviderCache with its separate header file, so it can include Nodes.h and have FunctionNode fully defined, and the use the standard std::unique_ptr and makeUnique.
ast-hugger
force-pushed
the
eng/JSC-Eagerly-build-AST-for-likely-IIFEs
branch
from
3c8578f to
535d24e
Compare
|
EWS run on previous version of this PR (hash 535d24e) Details
|
ast-hugger
force-pushed
the
eng/JSC-Eagerly-build-AST-for-likely-IIFEs
branch
from
535d24e to
7301bb0
Compare
|
EWS run on previous version of this PR (hash 7301bb0) Details
|
ast-hugger
force-pushed
the
eng/JSC-Eagerly-build-AST-for-likely-IIFEs
branch
from
7301bb0 to
71b8516
Compare
|
EWS run on previous version of this PR (hash 71b8516) Details |
macOS Safer C++ Build #98550 (71b8516)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
webkit-ews-buildbot
added
the
merging-blocked
iOS Safer C++ Build #16882 (71b8516)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
ast-hugger
removed
the
merging-blocked
ast-hugger
force-pushed
the
eng/JSC-Eagerly-build-AST-for-likely-IIFEs
branch
from
71b8516 to
cf77145
Compare
|
EWS run on previous version of this PR (hash cf77145) Details
|
| iifeFeatures = m_iifeParseState->features(); | ||
| iifeNumConstants = m_iifeParseState->numConstants(); | ||
| iifeLexFeatures = functionScope->lexicallyScopedFeatures(); | ||
| iifeInnerFeatures = functionScope->innerArrowFunctionFeatures(); | ||
| if (functionScope->shadowsArguments()) | ||
| iifeFeatures |= ShadowsArgumentsFeature; | ||
| if (functionScope->hasNonSimpleParameterList()) | ||
| iifeFeatures |= NonSimpleParameterListFeature; | ||
| if (m_seenTaggedTemplateInNonReparsingFunctionMode) | ||
| iifeFeatures |= NoEvalCacheFeature; | ||
| if (functionScope->usesImportMeta()) | ||
| iifeFeatures |= ImportMetaFeature; | ||
| if (m_seenArgumentsDotLength && functionScope->hasDeclaredGlobalArguments()) | ||
| iifeFeatures |= ArgumentsFeature; | ||
| if (functionScope->asyncFunctionBodyDoesNotUseAwait()) | ||
| iifeFeatures |= AsyncFunctionWithoutAwaitFeature; | ||
|
|
||
| iifeVarDeclarations = functionScope->declaredVariables(); | ||
| IdentifierSet capturedVariables; | ||
| functionScope->getCapturedVars(capturedVariables); | ||
| for (auto& entry : capturedVariables) | ||
| iifeVarDeclarations.markVariableAsCaptured(entry.get()); |
There was a problem hiding this comment.
Maybe these things can be unified with the normal Parser's final code? IIRC, we have similar code.
There was a problem hiding this comment.
Ok, I'll look into that.
ast-hugger
force-pushed
the
eng/JSC-Eagerly-build-AST-for-likely-IIFEs
branch
from
cf77145 to
2e6a77f
Compare
|
EWS run on current version of this PR (hash 2e6a77f) Details |
https://bugs.webkit.org/show_bug.cgi?id=311459 rdar://174062165 Reviewed by Yusuke Suzuki. This patch detects likely IIFEs (immediately invoked function expressions) and builds their AST right away, bypassing the usual initial syntax check. In general, we can't tell if a function expression is immediately invoked until after we parse it, but the following heuristic catches most real world cases. A function expression is expected to be an IIFE if the token preceding the `function` keyword is either `(` or `!` (`!` is commonly used by minifiers to introduce a function expression using one fewer character than wrapping it in parentheses). Another potential heuristic is if the `function` keyword is preceded by `,` and the expression before the comma was an IIFE. This heuristic is not used at this time because the cost/benefit tradeoff is not as clear in this case. Key changes: - A FunctionNode created when eagerly parsing an IIFE is independent from the AST created for the containing code and has its own lifetime. To support this, we change ParserArena and ParserArenaRoot so that an arena can be shared by multiple roots. A root no longer takes ownership of the arena memory, instead it retains a Ref to the arena. - Eager IIFE parsing is controlled by two classes, EagerIIFEParseScope and EagerIIFEParseState. The scope is created in Parser::parseFunctionInfo() when the IIFE prediction heuristic triggers. The scope contains an ASTBuilder to use for the IIFE and an EagerIIFEParseState. The state is registered with the parser as an indication of the special parsing mode it is currently in. It collects parsing artifacts such as function parameter and source elements until we are ready to create the final FunctionNode. When parseFunctionInfo exits, the scope is destroyed and its destructor returns the parser into the original state. - At the end of parseFunctionInfo, if an IIFE AST was eagerly built, we create a FunctionNode stored in m_eagerIIFERegistry. Parser::parse() checks the cache when the function is invoked and takes the ready-to-use AST from the cache. Testing: - Regression-checked by existing tests. - Eager parsing is not directly testable in stress tests. - Added `JSTests/stress/eager-iife-column-tracking.js` to test the source column adjustment required on the first line of IIFEs, as uncovered by one of the layout tests. Canonical link: https://commits.webkit.org/312444@main
webkit-commit-queue
force-pushed
the
eng/JSC-Eagerly-build-AST-for-likely-IIFEs
branch
from
2e6a77f to
709e4e7
Compare
|
Committed 312444@main (709e4e7): https://commits.webkit.org/312444@main Reviewed commits have been landed. Closing PR #62019 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
5 participants
709e4e7
2e6a77f