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

WIP proposal: debounce background store updates to UI #3535

Closed
wants to merge 1 commit into from

Conversation

Shadowfiend
Copy link
Contributor

@Shadowfiend Shadowfiend commented Jul 7, 2023

This is in many ways a much more targeted version of #1296 .

The observation feeding this PR is that the most expensive part of redux work right now is:

  • Diffing.
  • Serializing.
  • Deserializing.
  • Patching.

Doing this whole cycle on every action means that we can't structure actions freely, because we trigger the whole cycle on every action. We've experimented in the past with debouncing actions and subscribers at the cross-redux store level, but very quickly ran into edge cases with what happens on unsubscribe, timing issues, etc.

This approach eliminates all of that by only debouncing the state updates being pushed out by webext-redux. We do this by abusing the diff function on the background script side. The diff function now branches in two ways:

  • If we have no time for a next pending update, set a marker for when the next pending update should trigger, and trigger a noop action on the store to make sure the update goes out.
  • If we have a time for the next pending update and we haven't gotten to that time yet, return an empty diff.
  • If we have a time and we've crossed that time, clear the marker for the next pending update, and return the full state diff between old and new.

The diff only affects the UI states, not the background state—the background state is wholly managed by redux. The old state tracked by webext-redux only changes when the diff is non-empty. We use these two facts to create a single batch of changes at most every 300ms (hardcoded right now).

This commit needs a little cleanup and a message that looks more like this PR description.

Latest build: extension-builds-3535 (as of Fri, 07 Jul 2023 14:11:40 GMT).

@mhluongo
Copy link
Contributor

mhluongo commented Jul 8, 2023

Excited about this — decoupling the UI update performance issue from the data structure issue is 👌

@Shadowfiend
Copy link
Contributor Author

Unfortunately poked at it a bit and the improvements aren't what I wanted to see, and when things are performing smoothly the 300ms delay is noticeable (so we can't batch even more). Needs more digging, as always with performance stuff.

So far the biggest win I've seen has come from #3530, seemingly due to the cost of deep diffing arrays.

@Shadowfiend
Copy link
Contributor Author

Going to close this for now. Path seems like it could be promising, but measured outcomes seem less so.

@Shadowfiend Shadowfiend deleted the debounced-diffs branch July 22, 2023 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants