Location Pane redraw is slow for large files
Windows visual diff and merge for files and directories
Brought to you by:
christianlist,
grimmdp
Open a comparison of two large-ish (7MB) text files in WinMerge 2.6.8.0. The Location Pane will show that the comparison takes a while. Click Next Diff. The Location Pane forces a full comparison again! Turn off the Location Pane; Next Diff is now instantaneous.
Logged In: YES
user_id=631874
Originator: NO
It is known problem that location pane updates are slow. So it is not about new compare forced by location pane, just that location pane is slow to redraw. :(
For details, it location pane "diffs" are recalculated every time location pane is redrawn. For large files (or files with lots of diffs) it takes some time. Solution would be to determine the part needing redraw. For simple next/prev diff movements it would be changing color of the block. For merges it would be removing the block from list and bars.
I modified the summary to better describe the problem. Also increasing priority by one step.
Logged In: YES
user_id=631874
Originator: NO
While looking at location pane code, I noticed it does several linenumber lookups to compare views, per difference. E.g. in line 234-235 in current trunk LocationView.cpp. GetSubLineIndex() is certainly an expensive operation, especially done many times per difference. :(
There might not be easy way to solve this, but I have to look at it more closely.
Logged In: YES
user_id=631874
Originator: NO
Attached patch should help. Not a fix, but first move to correct direction.
First two hunks of the patch replace linenumber lookups with linenumber info from diffitem itself. They are equal, as diffitem has been synchronized with ghost (empty) lines earlier.
Last two hunks completely remove unneeded lookups.
File Added: speedup_location_pane.patch
Patch removing unnecessary line number lookups
Logged In: YES
user_id=631874
Originator: NO
I committed the patch (speedup_location_pane.patch) to SVN trunk.
Completed: At revision: 4374
Logged In: YES
user_id=954028
Originator: NO
I found that patch "speedup_location_pane.patch" causes incorrespondence between the diff marker and the visible area rectangle in word-wrap mode.
It seems that the patch works on only non-word-wrap mode.
Logged In: YES
user_id=631874
Originator: NO
Hmm. I tested the patch with word-wrap enabled also.
Anyway, I can revert the patch as it causes problems and I don't have time to track them now.
Ah, it actually makes sense, its the difference between mapped line numbers in file buffer and view's linenumbers after word-wrap. So much about my easy fix.
Logged In: YES
user_id=954028
Originator: NO
Here is the fix for the slow drawing.
When comparing the files that there are 50000 between the files, it is about 10 times faster than the old one. (0.3sec in OnDraw() -> 0.02sec)
The idea of this fix is to avoid drawing diff blocks to the same position.
--- C:/Documents and Settings/taka/Local Settings/Temp/LocationView.cpp-revBASE.svn001.tmp.cpp Wed Aug 08 22:57:29 2007
+++ C:/Dev/WinMergeDev/winmerge.org_svn2/trunk/Src/LocationView.cpp Wed Aug 08 22:55:09 2007
@@ -212,11 +212,16 @@
pDC->Rectangle(m_nLeftBarLeft, Y_OFFSET - 1, m_nLeftBarRight, nBottom);
pDC->Rectangle(m_nRightBarLeft, Y_OFFSET - 1, m_nRightBarRight, nBottom);
pDC->SelectObject(oldObj);
- for (int nDiff = 0; nDiff < pDoc->m_diffList.GetSize(); ++nDiff)
+
+ int nPrevEndY = -1;
+ const int nCurDiff = pDoc->GetCurrentDiff();
+ const int nDiffs = pDoc->m_diffList.GetSize();
+
+ for (int nDiff = 0; nDiff < nDiffs; ++nDiff)
{
DIFFRANGE diff;
VERIFY(pDoc->m_diffList.GetDiff(nDiff, diff));
@@ -226,34 +231,36 @@
}
- CMergeEditView *pView = m_view[MERGE_VIEW_LEFT];
-
// Count how many lines does the diff block have.
const int nBlockHeight = nLineEndDiff - diff.dbegin0;
- if (pDoc->GetCurrentDiff() == -1)
+ if (nCurDiff == -1)
DrawDiffMarker(pDC, -1);
- const BOOL bInsideDiff = pView->IsLineInCurrentDiff(diff.dbegin0);
+ const BOOL bInsideDiff = (nCurDiff == nDiff);
+ if (nPrevEndY != nEndY)
+ {
// Draw left side block
m_view[MERGE_VIEW_LEFT]->GetLineColors2(diff.dbegin0, ignoreFlags, cr0, crt, bwh);
CRect r0(m_nLeftBarLeft, nBeginY, m_nLeftBarRight, nEndY);
DrawRect(pDC, r0, cr0, bInsideDiff);
+ }
+ nPrevEndY = nEndY;
Logged In: YES
user_id=954028
Originator: NO
> 50000 between ...
50000 differences between ...
Logged In: YES
user_id=631874
Originator: NO
Ok, that looks like a good work-around at the moment. And it actually implements something I described in my first comment to this item. :)
However, I think we need to fix the fundamental design error for the location pane:
- location pane should keep its own list of diffs (maybe just begin- and end-linenumbers)
- redraw is always done based on that list
- list is lazily updated when user edits files (only when saving files?)
- when difference is merged, items are removed from list and removed items are cleared from GUI
- when active difference is changed, only GUI is updated (two block colors are changed)
That means we rarely redraw whole location pane, almost never re-calculate diffs and most changes are simple add/remove of differences and re-drawing only affected blocks.
But it needs some clever communication between file compare and location pane.
Btw, I just reverted my earlier fix. I assume it was still valid thing to do.
Logged In: YES
user_id=954028
Originator: NO
I agree with you. But the job is too much for me:)
>Btw, I just reverted my earlier fix. I assume it was still valid thing to
do.
FYI, your earlier fix also regenerated the following bug:
"[ 1584068 ] location pane is inaccurate when word wrap is on"
http://sourceforge.net/tracker/index.php?func=detail&aid=1584068&group_id=13216&atid=113216
Logged In: YES
user_id=631874
Originator: NO
Please commit your patch when you have time..
I've thought about rewriting that location pane redraw. But it isn't easy. And I haven't yet figured for example which way the data/messages should move:
- does location pane fetch updates from file compare periodically or
- does file compare send notifications about updates to location pane
The latter sounds the right way, but then location pane needs to tell file compare that user clicked it and wanted to move line yyy...
Logged In: YES
user_id=954028
Originator: NO
Committed to SVN trunk. Completed: At revision: 4407