Bug 228053 - DFGByteCodeParser.cpp should avoid resizing the Operands<> of every BasicBlock on every inlining
Summary: DFGByteCodeParser.cpp should avoid resizing the Operands<> of every BasicBloc...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on: 233387
Blocks:
  Show dependency treegraph
 
Reported: 2021-07-17 20:01 PDT by Robin Morisset
Modified: 2021-11-19 15:17 PST (History)
8 users (show)

See Also:


Attachments
Patch (13.79 KB, patch)
2021-07-17 20:06 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (13.92 KB, patch)
2021-07-17 20:09 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (13.91 KB, patch)
2021-07-17 20:11 PDT, Robin Morisset
sbarati: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (13.99 KB, patch)
2021-11-17 18:20 PST, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2021-07-17 20:01:48 PDT
The dfg bytecode parser only makes use of block->variablesAtTail. But currently it updates the size of variablesAtHead, valuesAtHead, valuesAtTail and intersectionOfPastValuesAtHead every single time it changes the number of Tmps and/or Locals. This happens notably whenever it inlines a function.
It is not nearly as cheap as it looks, as each resizing may reallocate a Vector, requires filling the new slots with zeros, and requires moving the existing values (which are all 0) to the new Vector. This was obvious when looking at profiling of JS2: bzero + memmove are the two hottest C++ functions, and the manipulation of Operands is partly responsible.
It can be easily improved: only resize block->variablesAtTail on inlining, and initialize all of the other operands at the very end of the DFGByteCodeParser.
Comment 1 Robin Morisset 2021-07-17 20:06:08 PDT
Created attachment 433740 [details]
Patch
Comment 2 Robin Morisset 2021-07-17 20:09:43 PDT
Created attachment 433742 [details]
Patch

updated Changelog with perf numbers.
Comment 3 Robin Morisset 2021-07-17 20:11:37 PDT
Created attachment 433743 [details]
Patch

Fix style nitpick.
Comment 4 Radar WebKit Bug Importer 2021-07-24 20:02:16 PDT
<rdar://problem/81064797>
Comment 5 Saam Barati 2021-11-17 15:57:14 PST
Comment on attachment 433743 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:8971
> +        block->variablesAtHead = Operands<Node*>(m_numArguments, m_numLocals, m_numTmps);
> +        block->valuesAtHead = Operands<AbstractValue>(m_numArguments, m_numLocals, m_numTmps);
> +        block->valuesAtTail = Operands<AbstractValue>(m_numArguments, m_numLocals, m_numTmps);
> +        block->intersectionOfPastValuesAtHead = Operands<AbstractValue>(m_numArguments, m_numLocals, m_numTmps);

nit: Use the OperandsLike constructor so these lines of code don't have to change in the future if we change the Operands constructor you're invoking?
Comment 6 Robin Morisset 2021-11-17 18:20:26 PST
Created attachment 444624 [details]
Patch

Applied Saam's suggestion and rebased
Comment 7 EWS 2021-11-18 14:57:05 PST
Committed r286030 (244418@main): <https://commits.webkit.org/244418@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444624 [details].
Comment 8 WebKit Commit Bot 2021-11-19 15:17:55 PST
Re-opened since this is blocked by bug 233387