Github: Staging and unstaging files feels unresponsive

[nathansobo]

Not sure this is due to shelling out to perform these actions, but I'm really noticing the lag when selecting a file to stage or unstage in the changed files list. I think we need to do something to make it feel more responsive... I favor making changes in the UI optimistically and rolling them back if there's an error. We could also show some kind of loading state but I don't think that's ideal.

[BinaryMuse]

We've made a bunch of changes to the async queue and the model observer to improve performance. Other than https://github.com/atom/github/issues/386, are you okay with closing this?

[smashwilson]

Based on some Slack conversations with @nathansobo I'm guessing the answer is "no" :wink: He's suggested that we target <100ms as an acceptable time window to produce a visible response to user input, in accordance with Google's RAIL performance model. We're currently hovering around ~500ms, and that'll just get worse on large repositories or slower machines.

Offhand I can't think of anything else but #382 that would reliably bring us under that time window - but I also think #382 is reasonably attainable for public launch if we scope it to staging and unstaging operations.

[smashwilson]

Just to brainstorm some other avenues to explore:

1. nodegit/libgit2 was already explored and was a no-go; see the discussions in #524 for details. Furthermore, in this case, the git status call in nodegit took much _longer_ than shelling out.
2. Selective invalidation of Repository state based on a finer-grained view of filesystem events is something we haven't really looked into. For example, there's no reason to re-read the current branch if .git/HEAD hasn't been touched. The downsides here are (a) the most expensive call, git status, is also the one we'd need most often anyway, and (b) filesystem events are noisy and occasionally lossy, so we'd risk missing an event and displaying stale data.
3. Re-implementing selected git operations in pure node could whittle away at the number of times we need to shell out. Unfortunately, status would be call we'd need the most, and it'd be a beast to port properly and keep consistent with real git.

[nathansobo]

I agree about focusing on staging and unstaging files and hunks and 100ms response being the target. Google has research backing up my intuition that it feels unsettling to interact in a fine-grained way with interfaces that don't update at least that fast. Even if we have to change the color of the hunk or something to confirm it being clicked, I really think we need some sort of response so staging doesn't feel like swimming in molasses.

[smashwilson]

Automatic refresh (#560) is in. It helps somewhat, but I've noticed that on Windows this is _really_ bad:

immediate-refresh.zip

The git apply --cached - operation there alone takes 120ms. I'll profile on my Mac on Monday to see how we're doing on that front.

671 is up next in this effort.

[BinaryMuse]

With https://github.com/atom/github/pull/633, and https://github.com/atom/github/pull/654, and https://github.com/atom/github/pull/654, we're down to right around *(and often under) 100ms for a stage/unstage-to-rerender time.

https://github.com/atom/github/pull/712 brings this down even more by allowing status calls to be parallelized, saving not only the amount of time it takes to execute status, but also the time lost by segregating the call to status away from other read operations (so sometimes up to ~30-50ms total).

This doesn't even include #700 yet, which will completely obviate the need to run certain commands at all in many cases.

[BinaryMuse]

@smashwilson Curious if you have a similar waterfall on the Windows machine you used ot generate the graph above?

[smashwilson]

@BinaryMuse Tomorrow's my Windows Day :tm:, so I'll collect a fresh waterfall tomorrow morning with all of the improvements :+1:

[smashwilson]

Here's a typical Windows trace for a stage-line operation on the current master (5bdca1d4):

master-stage-line.zip

Still not great but a good half-second faster :+1:

Also, just for comparison, here's a waterfall of the same operation with master merged in to #700:

700-stage-line.zip

Almost another half-second off, even with the doubled operations :tada:

[nathansobo]

Really appreciate the work y'all are doing to bring these times down. It will make a huge difference to the experience of staging in Atom.

[smashwilson]

700 is in! I've also added #727 to track a follow-on issue that's dampening its impact somewhat.

[smashwilson]

I'm moving this to _Complete_ on our ship project because we've shipped all the work we want to include in the public ship, but I'll leave the issue open for later prioritization of the caching improvements.