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

Improve redraw performance #948

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

R0nd
Copy link
Contributor

@R0nd R0nd commented Oct 16, 2021

This Pull Request fixes/closes #869.

It changes the following:

  • Improves redraw performance by moving sync repo operations off the UI thread

I followed the checklist:

  • I added unittests
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog

@extrawurst
Copy link
Owner

Does it make a noticeable difference? Did you do any measuring?

@R0nd
Copy link
Contributor Author

R0nd commented Oct 16, 2021

I tried scrolling through revlog continuously, and batching up to 10 input events was enough to clear the input event congestion (for a debug build on my setup)

@extrawurst
Copy link
Owner

I tried scrolling through revlog continuously, and batching up to 10 input events was enough to clear the input event congestion (for a debug build on my setup)

ok I was asking if you can see a noticeable difference though?

@extrawurst
Copy link
Owner

I checked out your branch. I cannot see any difference in a debug build compared to a debug build from master though. do you?

@R0nd
Copy link
Contributor Author

R0nd commented Oct 17, 2021

I've recorded screencasts do demonstrate the case - in both recordings I was holding the down arrow key until the revlog passed ~1400 commit number. The master build keeps processing the congested events long after the 1400 commit mark and the version with event batching stops scrolling just in time, showing the absence of congested events.
master
queue-updates

@R0nd
Copy link
Contributor Author

R0nd commented Oct 20, 2021

Honestly I think the only correct solution would be to move data fetching away from the UI thread, which would make this PR unnecessary. I'm new to rust, so I'm still trying to wrap my head around concurrency vs ownership.

@extrawurst
Copy link
Owner

Honestly I think the only correct solution would be to move data fetching away from the UI thread, which would make this PR unnecessary. I'm new to rust, so I'm still trying to wrap my head around concurrency vs ownership.

Not sure what you mean. What particular data fetching? Most expensive ones are run on a threadpool

@R0nd
Copy link
Contributor Author

R0nd commented Oct 20, 2021

The revlog update in particular, it runs a sync::get_commits_info on the UI thread. Which is the reason of #869 in the first place.

@extrawurst
Copy link
Owner

The revlog update in particular, it runs a sync::get_commits_info on the UI thread. Which is the reason of #869 in the first place.

well this is unlikely since this is only done once for every 1200 commits in a batch. did you measure this to be sure? if I only scroll inside the batch (so no get_commits_infocalls) it still lags. is that different in your case?

@R0nd
Copy link
Contributor Author

R0nd commented Oct 21, 2021

You're right, I've run profiling on this case and narrowed the issue down to update_commands checking the repo state TWICE on every redraw and also getting head in CommitComponent::can_amend
image
Maybe these values can be cached and updated only on git events?

@R0nd
Copy link
Contributor Author

R0nd commented Oct 21, 2021

Moving status updates off the UI thread seems to have worked out, redraw speed improved greatly and UI updates following repo status changes still work correctly, tested the case with "abort merge" command appearing after creating a merge commit.

@R0nd R0nd changed the title Input batching Improve redraw performance Nov 1, 2021
@extrawurst
Copy link
Owner

if we want to go forward with this we would need to bring it up to date with master

@extrawurst extrawurst marked this pull request as draft August 17, 2022 16:44
Copy link

stale bot commented Mar 17, 2024

This issue has been automatically marked as stale because it has not had any activity half a year. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the dormant Marked by stale bot on close label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dormant Marked by stale bot on close
Projects
None yet
Development

Successfully merging this pull request may close these issues.

decouple input from re-draw
2 participants