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

Interaction tracking ref-counting bug fixes (WIP) #13590

Merged
merged 4 commits into from
Sep 25, 2018

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Sep 7, 2018

Resolves #13574

  • 14c37f4: Added several additional edge case tests for suspending while sync rendering or after a placeholder has already timed out.
  • 2923bf5: Added a new test harness for myself in the future.
  • I also built the FB DOM renderer from this branch and verified that it fixed the original suspense repro case Royi submitted to me.

Most of the changes in this diff are test code. The actual fix is in fe9446f, within the following files:

@pull-bot
Copy link

pull-bot commented Sep 7, 2018

Details of bundled changes.

Comparing: 7bcc077...8c1cfd5

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% 0.0% 645.84 KB 646.31 KB 151.36 KB 151.43 KB UMD_DEV
react-dom.development.js +0.1% +0.1% 641.18 KB 641.65 KB 149.96 KB 150.04 KB NODE_DEV
ReactDOM-dev.js +0.1% +0.1% 663.57 KB 664.12 KB 152.32 KB 152.42 KB FB_WWW_DEV
react-dom.profiling.min.js +0.1% +0.1% 95.18 KB 95.27 KB 30.35 KB 30.38 KB NODE_PROFILING
ReactDOM-profiling.js +0.2% +0.1% 294.08 KB 294.68 KB 54.53 KB 54.59 KB FB_WWW_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.1% 439.5 KB 439.97 KB 98.51 KB 98.57 KB UMD_DEV
react-art.development.js +0.1% +0.1% 371.25 KB 371.72 KB 81.4 KB 81.47 KB NODE_DEV
ReactART-dev.js +0.1% +0.1% 376.26 KB 376.81 KB 80.17 KB 80.25 KB FB_WWW_DEV

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.1% +0.1% 383.25 KB 383.72 KB 83.97 KB 84.03 KB UMD_DEV
react-test-renderer.development.js +0.1% +0.1% 378.84 KB 379.31 KB 82.85 KB 82.92 KB NODE_DEV
ReactTestRenderer-dev.js +0.1% +0.1% 383.96 KB 384.51 KB 81.88 KB 81.96 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% +0.1% 367.16 KB 367.63 KB 79.52 KB 79.6 KB NODE_DEV
react-reconciler-persistent.development.js +0.1% +0.1% 365.7 KB 366.17 KB 78.94 KB 79.02 KB NODE_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.1% +0.1% 498.55 KB 499.1 KB 110.4 KB 110.49 KB RN_FB_DEV
ReactNativeRenderer-dev.js +0.1% +0.1% 498.28 KB 498.83 KB 110.33 KB 110.42 KB RN_OSS_DEV
ReactFabric-dev.js +0.1% +0.1% 488.73 KB 489.28 KB 107.98 KB 108.07 KB RN_FB_DEV
ReactFabric-dev.js +0.1% +0.1% 488.76 KB 489.32 KB 108 KB 108.09 KB RN_OSS_DEV
ReactNativeRenderer-profiling.js +0.3% +0.2% 212.24 KB 212.79 KB 37.19 KB 37.26 KB RN_OSS_PROFILING
ReactFabric-profiling.js +0.3% +0.2% 203.89 KB 204.43 KB 35.69 KB 35.77 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js +0.3% +0.2% 220.72 KB 221.33 KB 38.68 KB 38.74 KB RN_FB_PROFILING
ReactFabric-profiling.js +0.3% +0.2% 203.85 KB 204.39 KB 35.67 KB 35.75 KB RN_FB_PROFILING

schedule

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
schedule.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
schedule.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@bvaughn bvaughn requested a review from acdlite September 7, 2018 21:10
@bvaughn bvaughn force-pushed the interaction-ref-count-bugfix branch 3 times, most recently from f53138b to 845fccd Compare September 18, 2018 22:01
@bvaughn bvaughn requested review from acdlite and removed request for acdlite September 18, 2018 22:02
@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 18, 2018

Okay @acdlite, I think this is ready for another look.

// Do not remove the current interactions from the priority map on commit.
// This covers a special case of sync rendering with suspense.
// In this case we leave interactions in the Map until the suspended promise resolves.
let preservePendingInteractionsOnCommit: boolean = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This approach causes some interactions to get lumped together in the case of a suspended+sync root with a high priority interruption, but I think that might be acceptable?

I'd be happy to discuss alternatives!

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 25, 2018

Refactored the suspense logic based on our conversation today, and added some additional tests/fixtures @acdlite !

<div id="root"></div>
<!-- Load the tracing API before react to test that it's lazily evaluated -->
<script src="../../build/node_modules/schedule/umd/schedule.development.js"></script>
<script src="../../build/node_modules/schedule/umd/schedule-tracing.development.js"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs rebase? Name changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, poo. Ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebasing on master seems to suggest that master is broken 😄

$ yarn build

 BUILDING  react.development.js (umd_dev)
 OH NOES!  react.development.js (umd_dev)

Error: Could not resolve 'scheduler' from /Users/bvaughn/Documents/git/react/packages/react/src/ReactSharedInternals.js
    at /Users/bvaughn/Documents/git/react/node_modules/rollup-plugin-node-resolve/dist/rollup-plugin-node-resolve.cjs.js:85:23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I probably just need to run yarn first huh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always forget this silly step 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, rebased!

(scheduledInteractions, scheduledExpirationTime) => {
if (
earliestRemainingTimeAfterCommit === NoWork ||
scheduledExpirationTime < earliestRemainingTimeAfterCommit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe include a comment explaining why this check works, since it's a bit confusing.

@@ -260,7 +260,8 @@ let interruptedBy: Fiber | null = null;

// Do not decrement interaction counts in the event of suspense timeouts.
// This would lead to prematurely calling the interaction-complete hook.
let suspenseDidTimeout: boolean = false;
// Instead we wait until the suspended promise has resolved.
let interactionsHaveBeenSuspended: boolean = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: nextRenderIncludesTimedOutPlaceholder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This name isn't accurate for the initial null render but I don't care 😁

jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
* Added new (failing) suspense+interaction tests
* Add new tracing+suspense test harness fixture
* Refactored interaction tracing to fix ref counting bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interaction reference count decremented too aggressively
5 participants