search

facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
223.76k stars 45.56k forks source link

compiler: Use types to decide which scopes are eligible for merging #29157

Closed josephsavona closed 1 week ago

josephsavona commented 2 weeks ago

Stack from ghstack (oldest at bottom):

In MergeReactiveScopesThatInvalidateTogether when deciding which scopes were eligible for merging, we previously looked specifically at the type of the instruction whose lvalue is declaration. So if a scope declaration was t0, we'd look for the instruction where t0 was the lvalue and look at the instruction type to decide if it is eligible for merging.

Here, we use the inferred type instead (now that the inferred types support the same set of types of instructions we looked at before). This allows us to find more cases where scopes can be merged.

vercel[bot] commented 2 weeks ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 23, 2024 0:12am
react-sizebot commented 2 weeks ago

Comparing: bf046e86531141d0eb9ffc1165f775a57ffb00f5...6d4f5c8a3579108da5ccddffad17ceb1467a97a4

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.71 kB 495.71 kB = 88.78 kB 88.78 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 = 500.51 kB 500.51 kB = 89.47 kB 89.47 kB
facebook-www/ReactDOM-prod.classic.js = 592.86 kB 592.86 kB = 104.28 kB 104.28 kB
facebook-www/ReactDOM-prod.modern.js = 569.08 kB 569.08 kB = 100.69 kB 100.69 kB
__test_utils__/ReactAllWarnings.js Deleted 64.35 kB 0.00 kB Deleted 16.05 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](https://react-builds.vercel.app/commits/6d4f5c8a3579108da5ccddffad17ceb1467a97a4/files/__test_utils__/ReactAllWarnings.js?compare=bf046e86531141d0eb9ffc1165f775a57ffb00f5) | **Deleted** | 64.35 kB | 0.00 kB | Deleted | 16.05 kB | 0.00 kB

Generated by :no_entry_sign: dangerJS against 6d4f5c8a3579108da5ccddffad17ceb1467a97a4

josephsavona commented 1 week ago

Updated to add a test case confirming that the nested case memoizes as expected (ie does not inadvertently merge). I couldn't easily replicate that level of precision with useMemo, so i added a way to disable the non-forget execution in snap. This way we will get a test failure (not just a change to snapshot output) if we regress on the case you pointed out.