Improve inline spinner performance#5592
Merged
Merged
Conversation
7 tasks
stefanhaller
force-pushed
the
improve-inline-spinner-performance
branch
from
9f11291 to
6820bd2
Compare
Contributor
|
I'll run it for the rest of the day at work and do some additional tests tonight, I'll let you know how it goes (preliminary tests look good!) |
With a very slow spinner rate (seconds), you can see that at the beginning of an operation the bottom line leaves a gap for where the status will go, but the status (and spinner) is only drawn the first time the spinner ticks. The reason is that layout looks at GetStatusString to decide how much room to leave, rather than at the actual content of the AppStatus view; the view content is only set by renderAppStatus the first time the spinner ticks. Fix this by making layout look at the actual content of the view so that the layout is in sync with what is drawn. This also avoids flicker if an operation is so fast that it finishes before the spinner ticks for the first time, especially for users who set gui.showBottomLine to false.
stefanhaller
force-pushed
the
improve-inline-spinner-performance
branch
from
6820bd2 to
00ea9aa
Compare
Collaborator
Author
|
@AntoineGS I reordered the commits a bit and reworded some of them. Would be great if you could double-check that I didn't forget to pick anything from #5593. |
Contributor
|
Looks all there! |
…iews that overlap them This prevents views from drawing over higher z-order views. Currently this is not an issue in practice, because we use ForceFlushViewsContentOnly only for the bottom line status spinner, and there are never views on top of it. However, later in the branch we will use the mechanism to redraw the inline spinners in panels (e.g. the "Pushing..." status next to a branch name), and there could be a popup on top of it. Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
…hanges In 0d19507 we improved the performance of the status bar spinner by avoiding a layout. This is fine from one spinner tick to the next, but it's a problem when spinning starts or ends (or in the hypothetical case that the status text changes in the middle of the operation, which we never do in lazygit, but theoretically could). In this case a layout is needed so that the rest of the status bar gets pushed over appropriately (or moves back to the left when the spinner ends), and also so that the bottom line is shown or hidden properly for users who set gui.showBottomLine to false. To fix this, keep track of the status string width and force a layout whenever it changes. This includes the beginning and end of an operation when it changes from empty to non-empty or vice versa. There is currently no observable misbehavior from this bug, but that's only because we must have a HandleRender call somewhere that forces a full layout when an operation starts or ends. We will remove the Render() call from HandleRender at the end of this branch, at which point the misbehavior would be visible if we didn't fix it here.
Unrelated to this branch, just because we're touching this code: there's little reason to set the color on the background thread but the text on the UI thread. Set them both together on the UI thread. Avoids a data race (unlikely to be a problem in practice, we're talking about a 64-bit int, but still).
An async refresh dispatches refreshXyz on a worker goroutine, which then calls refreshView -> PostRefreshUpdate -> HandleRender. Today the final self.c.Render() inside ListContextTrait.HandleRender is what triggers a UI flush from the worker. We're going to remove that Render() call, so prepare by wrapping refreshView's body in OnUIThread. This moves the entire rendering of the view (and the ReApplyFilter/ReApplySearch stuff) to the UI thread, not just the layout. I don't expect this to make a difference in practice, and it is already one step towards my long-term goal of moving all view rendering to the UI thread (see #2974 (comment)).
refreshBranches runs on a worker goroutine and re-renders the commits view directly to refresh the branch-head visualization. As with refreshView, this currently only flushes because HandleRender ends with self.c.Render(). Wrap the explicit HandleRender call (along with the LocalCommitsMutex pair around it) in OnUIThread so it keeps working once Render() is removed.
SetSuggestions has two callers: prepareConfirmationPanel calls it directly on the UI thread, while editors.promptEditor and SuggestionsContext.RefreshSuggestions call it via AsyncHandler, which runs the result closure on a worker goroutine. The worker path currently relies on HandleRender's self.c.Render() to flush the view update. Wrap the body in OnUIThread so the worker path stays correct when Render() is removed; for the UI-thread caller the extra bounce is harmless.
The redraw of the selection color (using <space>) would be tied to the spinner drawing. To reproduce, having a high spinner refresh rate and toggling a file would see a delay equivalent to the time spinner refresh rate.
Fixes an issue mostly noticeable using the go -race mode, this resolves them.
self.c.Render() at the end of HandleRender was there to schedule a gocui Update tick so the view content modified above would actually get drawn. For UI-thread callers (the great majority -- keybinding handlers, the layout function itself, popup resize, etc.) this was unnecessary work, since gocui already runs a layout/redraw cycle after every event. SimpleContext.HandleRender doesn't call Render() either, so this aligns the two implementations. The few callers that drove HandleRender from a worker goroutine and relied on Render() for the flush were wrapped in OnUIThread in the preceding commits, so the implicit Render is no longer needed. Also, Render() being called *before* setFooter() looks like it might have been a theoretical race; this is no longer an issue now.
Now that HandleRender no longer does an implicit Render(), we can use it inside OnUIThreadContentOnly to save performance, on the assumption that rerendering a view that contains an inline spinner never changes the layout.
stefanhaller
force-pushed
the
improve-inline-spinner-performance
branch
from
00ea9aa to
4734bab
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As a followup to #5571, improve performance for inline spinners too.
This requires a bit of preparation; in particular, we change ListContextTrait.HandleRender to no longer force a UI layout/redraw, so callers need to take care of this themselves where needed. This requires careful testing to see that we didn't miss any situation where redrawing only worked by accident; see the first commit of the branch for an example.