Skip to content

optimize and reduce the time spent computing ahead/behind info#4399

Merged
iAmWillShepherd merged 24 commits intocompare-branchesfrom
crunch-time
Apr 9, 2018
Merged

optimize and reduce the time spent computing ahead/behind info#4399
iAmWillShepherd merged 24 commits intocompare-branchesfrom
crunch-time

Conversation

@shiftkey
Copy link
Member

@shiftkey shiftkey commented Apr 5, 2018

Fixes #4399

Some changes to reduce the work that Desktop does for crunching the ahead/behind counts:

  • a unified ComparisonCache which uses the two refs as a lookup key - simplifies the app store work
  • aborting inflight operations when quickly switching branches
  • use requestIdleCallback to prioritize this work behind everything else
  • confirm scenarios where the tip changes are triggering crunching
    • after making a new commit
    • after pulling
    • after a merge
  • cleanup isCrunching state as this isn't intended to ship as a UI feature

There's also a visual cue to indicate when it's working to help with debugging on my end, but I'll pull this out before merging.

@shiftkey
Copy link
Member Author

shiftkey commented Apr 6, 2018

I've finally been able to get most of the hard stuff isolated into this AheadBehindUpdater module, so I'll summarise the current flow of information around the Compare tab:

  • selecting a repository sets up a AheadBehindUpdater to process and cache ahead/behind data
  • inside AheadBehindUpdater it uses a queue (using the creatively-named queue package) to process actions in a first-in, first-out fashion
  • the user performs some action that means we're probably interested in ahead/behind stats, triggering the AppStore to update it's state
  • the AppStore passes the current AheadBehindUpdater a set of refs that need to be computed
  • the AheadBehindUpdater ditches any in-flight work (basically because it's associated with an old HEAD) and queues up a number of tasks (using its cache to skip work it already knows is done)
  • these tasks are processed serially by the queue, with each being prioritized with a requestIdleCallback to ensure it doesn't take away from anything else happening in the UI
  • when a task is done, AheadBehindUpdater will signal back to the AppStore that it has some new values to use in the Compare tab

Things that are nice

  • a unified ComparisonCache that tracks all ahead/behind stats for a repository avoids needing to depend on HEAD in a repository - there's now a risk that we might cache too much for large repos, but at least we now have one place to clean this up
  • using the *Updater pattern like we've done with other components means we can push the complexity about scheduling work out of AppStore
  • this strikes a nice balance between background work (using requestIdleCallback) and updating the UI (by emitting updates in AppStore) - this means if you're browsing a long list of refs the UI remains snappy, but will populate the ahead/behind stats as they become available. Here's a GIF:

Things that need work

  • I'm still enabling other scenarios where HEAD changes under us and we need to update the compare view, being careful to ensure the background work doesn't interfere with the rest of the app. That's next.
  • using ComparisonCache directly in AppState and then signalling every time it's updated feels excessive. It'd be great to just attach this to the branches we already track in our list, but I don't want to affect other parts of the codebase. Deferring thinking about this more until I've addressed other optimizations.

@shiftkey shiftkey added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 6, 2018
@shiftkey shiftkey requested a review from iAmWillShepherd April 6, 2018 06:52
@shiftkey shiftkey added this to the 1.2.0 milestone Apr 6, 2018
@j-f1
Copy link
Contributor

j-f1 commented Apr 8, 2018

Would it be possible to run multiple ahead-behind checks at the same time so results show up faster?

@shiftkey
Copy link
Member Author

shiftkey commented Apr 8, 2018

Would it be possible to run multiple ahead-behind checks at the same time so results show up faster?

The current limit to concurrency is spawning Git processes - we have to spawn N Git operations to get the ahead/behind counts for N branches because rev-list only supports providing one range. Command line examples of this behaviour are just shell scripts wrapping this pattern.

Now, we could batch the onCacheUpdate calls so that they run N/3 times (so signal for each 3 calls) or something to avoid excessively re-rendering, but I think that's not hugely important here - and easy to adjust now we've got the core in place.

Copy link
Contributor

@iAmWillShepherd iAmWillShepherd left a comment

Choose a reason for hiding this comment

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

I don't want to hold up this PR with the things, I've mentioned. So I'm gonna make the changes and merge.

} comparisons to perform`
)

if (newRefsToCompare.size === 0) {

This comment was marked as spam.


public get(from: string, to: string) {
const key = ComparisonCache.getKey(from, to)
return this.backingStore.get(key)

This comment was marked as spam.

iAmWillShepherd added 2 commits April 9, 2018 10:56
The whole point of this component is to show ahead/behind info, let’s require a value for that prop
@iAmWillShepherd iAmWillShepherd merged commit e181ae5 into compare-branches Apr 9, 2018
@iAmWillShepherd iAmWillShepherd deleted the crunch-time branch April 9, 2018 16:12
@shiftkey shiftkey mentioned this pull request Apr 23, 2018
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants