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

Speedup browsing huge patches - show changes for single files. #125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

agutikov
Copy link
Contributor

@agutikov agutikov commented Feb 21, 2022

With large patches next functions of PatchContent became so slow that app became unresponsive:

  • setPlainText() - because of highlighting, not so slow, but significant
  • find()
  • moveCursor(QTextCursor::End)

And if you select revision with huge patch in RevsView app eats 100% cpu core and freeze for seconds to minutes.
Then if you select file from FileList - then it hangs almost forever.

This patch makes PatchContent show changes only for selected file if size of patch text exceeds configured limit.

Added configuration option "Max patch size MiB".
If size of patch text exceeds that limit - PatchContent splits it into per-file change
and shows changes for each selected file separately.
Otherwise - no changes, will be shown entire patch content.

…xt exceeds configured limit.

Added configuration option "Max patch size MiB".
If size of patch text exceeds that limit - PatchContent splits it into per-file change
and shows changes for each selected file separately.
Otherwise - no changes, will be shown entire patch content.
@tibirna
Copy link
Owner

tibirna commented Feb 26, 2022

Thank you for the useful functionality.

A few issues:

  • the setting "Max patch size MiB" defaults to 0, forcing the new splitting per file to be the default behaviour. Please select a default larger than 0. The previous (non splitting) behaviour must be the default one.
  • the explanation "Bigger patches will be shown by-file", which is a crucial bit of information, is hidden in an invisible tooltop
  • the user would not have any idea what is a good value for the new setting. I think the better UI should rather offer a simple checkbox "Display very large patches split per file". The minimum size for a "big diff" should be deduced from CPU characteristics, or, at the least, an arbitrary hidden constant should be selected, based on some heuristics. The value can be hardwired in the code, no need to expose it to the user.
  • when the functionality is active, the "Diff" view only shows the list of changed files. This is surprising. The user is not informed in any way how to get to the actual diff. I would recommend adding a message in the "Diff" display to the effect of "Large commit, diffs are displayed per file. Select a file to see its changes".

Please, give an example of a repository containing very big diffs, on which I could inspect the observed slowness issue.

Thanks again
Cristian

@WickedSmoke
Copy link
Contributor

I run into this bug occasionally so I've applied a simplified version of this patch (without settings configuration) to my repository with a hardcoded 400KiB limit. At that size the pause is a couple seconds on my system.

This repository has a variety of large commits that can be used for testing: https://github.com/huxingyi/dust3d

commit 8e5d622db78c00f5c9c1b7b1ad9d13985c7cc531 
Date:   Thu Nov 18 22:58:01 2021 +0800
    Restructure the code base
10390 files changed

commit 5b7c9dbc448079b3e0d21a9b046d8def4bdfc9e8
Date:   Tue Oct 13 22:14:25 2020 +0930
    Update CGAL to 5.1
5222 files changed

commit 8c6a9fef29684884dca8d9d2f11adefd907b1188
Date:   Sat Jan 4 22:19:03 2020 +0930
    Mark remeshed component as Uncombined
7002 files changed

commit 3b4d4ddc52e5cdfecedac7523b185073e95d403e
Date:   Tue Dec 31 21:16:33 2019 +0930
    Update libigl and eigen
2012 files changed

commit dd628b6af7f1b425c128fd0ef59a4bada17fa234
Date:   Sat Dec 28 14:51:59 2019 +0930
    Integrate QuadriFlow
283 files changed

commit 17cb1cb0599f2514fba5cc3147ff0dade0dfbe14
Date:   Sun Sep 22 00:00:27 2019 +0930
    Introduce bullet3 to support ragdoll procedural animation
702 files changed

commit 208d2a0166f34de8073cb57eba5be9fea09c1f5e
Date:   Thu Oct 25 08:19:38 2018 +0800
    Cleanup
135 files changed

commit a4a734a67b6bad10fc7005534976e6259e019ad4
Date:   Mon Oct 15 23:02:31 2018 +0800
    Repleace the uv unwrapping library with simpleuv
1533 files changed

commit 09d21e893527454b56d7fcfb0a832c9a2dd2073a
Date:   Mon Mar 26 20:41:46 2018 +0800
    Add skeleton snapshot support (Prepare for undo/redo feature)
1172 files changed

@tibirna
Copy link
Owner

tibirna commented Jul 7, 2022

Hello Karl

Thanks for the test repo example and for the report on the behaviour of your patch.

Alexei,

Do you have comments on my previous notes from the reply of 2022-02-25? I don't see any additional changes in your forked repo (although you mention a simplified version above). Am I looking in the wrong place?

@WickedSmoke
Copy link
Contributor

The change was to my local repo, but I've now pushed it to https://github.com/WickedSmoke/qgit/tree/large_patch. Note that patchcontent.h is unchanged from Aleksei's code, so the comments there are not accurate.

Ideally this would be fixed in a way that prevents the slowdown from ever occurring and without any work-around settings. It may need to be considered along with Log/Diff tab changes of #44, #85, & #105. Other Git browsers handle this by keeping all diffs in one view, but collapsing the diff of individual files which are too large. This would work with my preferred change of eliminating the tabs.

Note that a short term solution of using my hardcoded largePatchBytes along with your recommend message in the "Diff" display would make this initial display unreachable after clicking on a file. The user would have to select another commit and re-select the desired one to see the list of changes again.

@WickedSmoke
Copy link
Contributor

I updated my large_patch branch with a "[Large Patch]" notice. Even with the caveat about the unreachable initial display, I think pushing something similar to master is preferable to the current behavior which makes the program unusable.

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.

3 participants