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

Deterministic updates #10715

Merged
merged 10 commits into from
Oct 14, 2017
Merged

Deterministic updates #10715

merged 10 commits into from
Oct 14, 2017

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Sep 15, 2017

High priority updates typically require less work to render than low priority ones. It's beneficial to flush those first, in their own batch, before working on more expensive low priority ones. We do this even if a high priority is scheduled after a low priority one.

However, we don't want this reordering of updates to affect the terminal state. State should be deterministic: once all work has been flushed, the final state should be the same regardless of how they were scheduled.

To get both properties, we store updates on the queue in insertion order instead of priority order (always append). Then, when processing the queue, we skip over updates with insufficient priority. Instead of removing updates from the queue right after processing them, we only remove them if there are no unprocessed updates before it in the list.

This means that updates may be processed more than once.

As a bonus, the new implementation is simpler and requires less code.

To avoid conflicts, this works off my expiration times (#10426) and prerendering (#10624) PRs.

@acdlite acdlite force-pushed the deterministicupdates branch 7 times, most recently from 2d46bad to 20eb374 Compare September 15, 2017 20:45
@acdlite acdlite force-pushed the deterministicupdates branch 2 times, most recently from e2ab6b9 to d47de1e Compare September 29, 2017 07:30
@reactjs-bot
Copy link

Deploy preview ready!

Built with commit e2ab6b9

https://deploy-preview-10715--reactjs.netlify.com

@reactjs-bot
Copy link

reactjs-bot commented Sep 29, 2017

Deploy preview ready!

Built with commit d0d798e

https://deploy-preview-10715--reactjs.netlify.com

@acdlite acdlite force-pushed the deterministicupdates branch 10 times, most recently from 9a31c91 to d0d798e Compare October 5, 2017 18:30
@acdlite acdlite force-pushed the deterministicupdates branch 4 times, most recently from d70d52e to e3debd7 Compare October 9, 2017 17:10
props: mixed,
renderExpirationTime: ExpirationTime,
): State {
): State | null {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can this return null now?

) {
const coalescedTime = insertAfter.expirationTime;
queue.expirationTime = update.expirationTime;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you expand the if (queue.last === null) { block to include the else condition instead of doing an early return, we can reuse this logic for both branches instead of duplicating.

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.

6 participants