Skip to content
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

compiler: merge reactive scopes across const StoreLocal #29095

Merged

Conversation

josephsavona
Copy link
Contributor

@jbonta nerd-sniped me into making this optimization during conference prep, posting this as a PR now that keynote is over.

Consider these two cases:

export default function MyApp1({ count }) {
  const cb = () => count;
  return <div onclick={cb}>Hello World</div>;
}

export default function MyApp2({ count }) {
  return <div onclick={() => count}>Hello World</div>;
}

Previously, the former would create two reactive scopes (one for cb, one for the div) while the latter would only have a single scope for the div and its inline callback. The reason we created separate scopes before is that there's a StoreLocal 'cb' = t0 instruction in-between, and i had conservatively implemented the merging pass to not allow intervening StoreLocal instructions.

The observation is that intervening StoreLocals are fine if the assigned variable's last usage is the next scope. We already have a check that the intervening lvalues are last-used at/before the next scope, so it's trivial to extend this to support StoreLocal.

Note that we already don't merge scopes if there are intervening terminals, so we don't have to worry about things like conditional StoreLocal, conditional access of the resulting value, etc.

@josephsavona josephsavona requested a review from mofeiZ May 16, 2024 06:53
@josephsavona josephsavona requested a review from poteto May 16, 2024 06:53
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label May 16, 2024
@react-sizebot
Copy link

react-sizebot commented May 16, 2024

Comparing: cf7d895...81220c3

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 495.01 kB 495.01 kB = 88.68 kB 88.68 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 499.81 kB 499.81 kB = 89.36 kB 89.36 kB
facebook-www/ReactDOM-prod.classic.js = 592.16 kB 592.16 kB = 104.15 kB 104.15 kB
facebook-www/ReactDOM-prod.modern.js = 568.39 kB 568.39 kB = 100.55 kB 100.55 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Generated by 🚫 dangerJS against 81220c3

Copy link
Member

@poteto poteto left a comment

Choose a reason for hiding this comment

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

nice! looks a lot cleaner too

@josephsavona josephsavona force-pushed the merge-scopes-across-storelocal branch from cb8ef0a to 81220c3 Compare May 17, 2024 00:27
@josephsavona josephsavona merged commit 5ab5471 into facebook:main May 17, 2024
11 of 34 checks passed
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.

4 participants