Skip to content

Prevent inlining into recursive commit functions #20105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Oct 27, 2020

Adds a bunch of no-inline directives to commit phase functions to prevent them from being inlined into one of our recursive algorithms.

The motivation is to minimize the number of variables in the recursive functions, since each one contributes to the size of the stack frame.

Theoretically, this could help the performance of both the recursive and non-recursive (iterative) implementations of the commit phase, since even the iterative implementation sometimes uses the JS stack.

@acdlite acdlite changed the title Prevent inlining commit phase Prevent inlining into recursive commit functions Oct 27, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 27, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7819cd3:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Oct 27, 2020

Details of bundled changes.

Comparing: 25b18d3...7819cd3

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% -0.0% 872.67 KB 872.67 KB 199.75 KB 199.75 KB NODE_DEV
ReactDOMForked-prod.js -0.4% -0.2% 387.21 KB 385.73 KB 71.42 KB 71.26 KB FB_WWW_PROD
react-dom-server.node.development.js 0.0% 0.0% 137.05 KB 137.05 KB 36.37 KB 36.37 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 117.75 KB 117.75 KB 37.95 KB 37.95 KB NODE_PROD
ReactDOMForked-profiling.js -0.3% -0.1% 403.35 KB 402.2 KB 74.08 KB 73.98 KB FB_WWW_PROFILING
react-dom-test-utils.development.js 0.0% 0.0% 66.27 KB 66.27 KB 18.83 KB 18.83 KB NODE_DEV
ReactDOMTesting-prod.js 0.0% 0.0% 384.74 KB 384.74 KB 72.38 KB 72.38 KB FB_WWW_PROD
react-dom.production.min.js 0.0% 0.0% 117.65 KB 117.65 KB 38.64 KB 38.64 KB UMD_PROD
react-dom.profiling.min.js 0.0% 0.0% 121.55 KB 121.55 KB 39.85 KB 39.85 KB UMD_PROFILING
ReactDOMForked-dev.js 0.0% 0.0% 995.88 KB 996.1 KB 219.86 KB 219.89 KB FB_WWW_DEV
react-dom.profiling.min.js 0.0% 0.0% 121.83 KB 121.83 KB 39.13 KB 39.13 KB NODE_PROFILING
ReactDOM-prod.js 0.0% 0.0% 381.46 KB 381.46 KB 70.32 KB 70.32 KB FB_WWW_PROD
react-dom-server.browser.production.min.js 0.0% 0.0% 19.77 KB 19.77 KB 7.42 KB 7.42 KB NODE_PROD
ReactDOMServer-dev.js 0.0% 0.0% 145.51 KB 145.51 KB 37.29 KB 37.29 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% -0.0% 47.29 KB 47.29 KB 11.03 KB 11.03 KB FB_WWW_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 7819cd3

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Oct 27, 2020
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is fine?

Seems like the sort of thing we should be measuring before and after and seeing what the impact is. I don't have an intuition about whether this would help, hurt, or be neutral.

@sizebot
Copy link

sizebot commented Oct 27, 2020

Details of bundled changes.

Comparing: 25b18d3...7819cd3

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% -0.0% 908.22 KB 908.22 KB 206.3 KB 206.3 KB NODE_DEV
ReactDOMForked-prod.js -0.4% -0.2% 375.89 KB 374.41 KB 69.63 KB 69.47 KB FB_WWW_PROD
react-dom.production.min.js 0.0% 0.0% 122.29 KB 122.29 KB 39.3 KB 39.3 KB NODE_PROD
ReactDOMForked-profiling.js -0.3% -0.1% 391.97 KB 390.83 KB 72.3 KB 72.2 KB FB_WWW_PROFILING
react-dom-server.browser.development.js 0.0% 0.0% 144.72 KB 144.72 KB 36.76 KB 36.76 KB UMD_DEV
react-dom-test-utils.development.js 0.0% 0.0% 66.28 KB 66.28 KB 18.84 KB 18.84 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 5.52 KB 5.52 KB 1.84 KB 1.84 KB NODE_DEV
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 4.78 KB 4.78 KB 1.68 KB 1.68 KB NODE_DEV
react-dom.profiling.min.js 0.0% 0.0% 127.38 KB 127.38 KB 41.65 KB 41.65 KB UMD_PROFILING
ReactDOMForked-dev.js 0.0% 0.0% 970.3 KB 970.52 KB 215.08 KB 215.11 KB FB_WWW_DEV
react-dom.profiling.min.js 0.0% 0.0% 127.73 KB 127.73 KB 40.95 KB 40.96 KB NODE_PROFILING
react-dom-server.browser.production.min.js 0.0% 0.0% 20.33 KB 20.33 KB 7.55 KB 7.55 KB UMD_PROD
ReactDOM-dev.js 0.0% 0.0% 955.16 KB 955.16 KB 213.31 KB 213.31 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 0.0% 370.21 KB 370.21 KB 68.54 KB 68.54 KB FB_WWW_PROD
react-dom-server.browser.development.js 0.0% 0.0% 137.3 KB 137.3 KB 36.33 KB 36.33 KB NODE_DEV
ReactDOMServer-dev.js 0.0% 0.0% 141.48 KB 141.48 KB 36.28 KB 36.28 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% 0.0% 46.43 KB 46.43 KB 10.82 KB 10.82 KB FB_WWW_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against 7819cd3

Adds a bunch of no-inline directives to commit phase functions to
prevent them from being inlined into one of our recursive algorithms.

The motivation is to minimize the number of variables in the recursive
functions, since each one contributes to the size of the stack frame.

Theoretically, this could help the performance of both the recursive
and non-recursive (iterative) implementations of the commit phase,
since even the iterative implementation sometimes uses the JS stack.
@acdlite acdlite force-pushed the prevent-inlining-commit-phase branch from f21d9e1 to 7819cd3 Compare October 27, 2020 19:07
@acdlite
Copy link
Collaborator Author

acdlite commented Oct 27, 2020

Seems like the sort of thing we should be measuring before and after and seeing what the impact is.

Agree though since this is a build-time optimization, will need to use the reconciler fork infra for that. So we need to land the new reconciler first.

In the meantime, we can run a local test.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 27, 2020

I don't have a lot of faith in our local tests (since we haven't even been able to catch anything resembling a perf regression that could explain the top line metrics regression)

@acdlite acdlite merged commit 779a472 into facebook:master Oct 27, 2020
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Nov 9, 2020

Verified

This commit was signed with the committer’s verified signature. The key has expired.
weihanglo Weihang Lo
Summary:
Base sync before adding Flight files.

This sync includes the following changes:
- **[454c2211c](facebook/react@454c2211c )**: Refactor SchedulerHostConfigs ([#20025](facebook/react#20025)) //<Ricky>//
- **[56e9feead](facebook/react@56e9feead )**: Remove Blocks ([#20138](facebook/react#20138)) //<Sebastian Markbåge>//
- **[3fbd47b86](facebook/react@3fbd47b86 )**: Serialize pending server components by reference (lazy component) ([#20137](facebook/react#20137)) //<Sebastian Markbåge>//
- **[930ce7c15](facebook/react@930ce7c15 )**: Allow values to be encoded by "reference" to a value rather than the value itself ([#20136](facebook/react#20136)) //<Sebastian Markbåge>//
- **[39eb6d176](facebook/react@39eb6d176 )**: Rename ([#20134](facebook/react#20134)) //<Sebastian Markbåge>//
- **[ffd842335](facebook/react@ffd842335 )**: [Flight] Add support for Module References in transport protocol ([#20121](facebook/react#20121)) //<Sebastian Markbåge>//
- **[343d7a4a7](facebook/react@343d7a4a7 )**: Fast Refresh: Don't block DevTools commit hook ([#20129](facebook/react#20129)) //<Brian Vaughn>//
- **[779a472b0](facebook/react@779a472b0 )**: Prevent inlining into recursive commit functions ([#20105](facebook/react#20105)) //<Andrew Clark>//
- **[25b18d31c](facebook/react@25b18d31c )**: Traverse commit phase effects iteratively ([#20094](facebook/react#20094)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions 4e5d7fa...454c221

Reviewed By: rickhanlonii

Differential Revision: D24698701

fbshipit-source-id: dfaf692b1051150355dece1657764a484b7ae603
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Adds a bunch of no-inline directives to commit phase functions to
prevent them from being inlined into one of our recursive algorithms.

The motivation is to minimize the number of variables in the recursive
functions, since each one contributes to the size of the stack frame.

Theoretically, this could help the performance of both the recursive
and non-recursive (iterative) implementations of the commit phase,
since even the iterative implementation sometimes uses the JS stack.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants