Menu

#1519 Location Pane redraw is slow for large files

open
nobody
Usability (119)
6
2007-07-04
2007-07-03
Jack Tanner
No

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.

Discussion

  • Kimmo Varis

    Kimmo Varis - 2007-07-04
    • labels: --> Usability
    • priority: 5 --> 6
    • summary: Location Pane causes slowdown on large files --> Location Pane redraw is slow for large files
     
  • Kimmo Varis

    Kimmo Varis - 2007-07-04

    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.

     
  • Kimmo Varis

    Kimmo Varis - 2007-07-18

    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.

     
  • Kimmo Varis

    Kimmo Varis - 2007-07-18

    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

     
  • Kimmo Varis

    Kimmo Varis - 2007-07-18

    Patch removing unnecessary line number lookups

     
  • Kimmo Varis

    Kimmo Varis - 2007-07-20

    Logged In: YES
    user_id=631874
    Originator: NO

    I committed the patch (speedup_location_pane.patch) to SVN trunk.
    Completed: At revision: 4374

     
  • Takashi Sawanaka

    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.

     
  • Kimmo Varis

    Kimmo Varis - 2007-08-01

    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.

     
  • Takashi Sawanaka

    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);

    // Iterate the differences list and draw differences as colored blocks.
    

    - 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));

        // Skip trivial differences.
    

    @@ -226,34 +231,36 @@
    }

        // Find end of diff. If first side has blank lines use other side.
        const int nLineEndDiff = \(diff.blank0 &gt; 0\) ? diff.dend1 : diff.dend0;
    

    - CMergeEditView *pView = m_view[MERGE_VIEW_LEFT];
    -
    // Count how many lines does the diff block have.
    const int nBlockHeight = nLineEndDiff - diff.dbegin0;

        // Convert diff block size from lines to pixels.
        const int nBeginY = \(int\)\(diff.dbegin0 \* LineInPix + Y\_OFFSET\);
        const int nEndY = \(int\)\(\(diff.dbegin0 + nBlockHeight\) \* LineInPix + Y\_OFFSET\);
    
        // If no selected diff, remove diff marker
    

    - 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);

        // Draw right side block
        m\_view\[MERGE\_VIEW\_RIGHT\]-&gt;GetLineColors2\(diff.dbegin0, ignoreFlags, cr1, crt, bwh\);
        CRect r1\(m\_nRightBarLeft, nBeginY, m\_nRightBarRight, nEndY\);
        DrawRect\(pDC, r1, cr1, bInsideDiff\);
    

    + }
    + nPrevEndY = nEndY;

        // Test if we draw a connector
        BOOL bDisplayConnectorFromLeft = FALSE;
        BOOL bDisplayConnectorFromRight = FALSE;
    
     
  • Takashi Sawanaka

    Logged In: YES
    user_id=954028
    Originator: NO

    > 50000 between ...
    50000 differences between ...

     
  • Kimmo Varis

    Kimmo Varis - 2007-08-08

    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.

     
  • Takashi Sawanaka

    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

     
  • Kimmo Varis

    Kimmo Varis - 2007-08-08

    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...

     
  • Takashi Sawanaka

    Logged In: YES
    user_id=954028
    Originator: NO

    Committed to SVN trunk. Completed: At revision: 4407

     

Log in to post a comment.