#1411 Allow caller to skip scrollbar updates and repainting during replace all and related operations

[Nick Powell]

Replace all in SciTE and Notepad++ is currently extremely slow when the replacements insert or delete lines in the document. I did some profiling and found that ~95% of this time is spent updating scrollbar positions in the Editor class. My machine runs windows 10, so the Windows API call seems to be the slow part here.

I've attached a proof of concept diff that makes replace all in SciTE >100x faster on my machine by allowing the caller to suspend visual updates in the Editor class when they expect to do a lot of modifications at once (Re-using logic from the MultiStepUndoRedo flag). In SciTEBase::DoReplaceAll, I just call wEditor.BeginBatchModification() before entering the main loop and wEditor.EndBatchModification() after.

My test was to replace all instances of \n with \n\n on the world192.txt file from here. The time taken went from 17,669ms to 154ms

I'm happy to write a proper patch for this if you think the change is worthwhile

[Neil Hodgson]

This doesn't appear to be safe for a multiple view scenario since the end of batching isn't broadcast to all views. No views received updates during the batch.

The viewed text may move up/down if earlier content is changed. This may not be expected by the caller who may then need to scroll to preserve stability.

It may be possible to scope modifications more tightly to the scroll bars by stopping just the scroll bar system calls during a batch. I don't know if that would eliminate all the overhead and I have never managed to find time to implement it. There are 4 main pieces of modifiable data for each scroll bar: position, maximum position, page size, and visibility/enabled. Depending on platform, step increment (arrow press) and page increment (press background) may also be API values. On batch start: copy scroll bar values from system; modifications go into copies without calling system APIs, queries are answered from copies; on batch end: call system APIs to match copies. Batches must finish synchronously before user can possibly interact with scroll bars.

[Nick Powell]

Ah, I thought there might be some non-obvious side effects

It seems like it the first issue could be solved by sending a new event type from the Document to mark the end of the batch. Any watcher that isn't interested in this optimisation can just ignore it after all

For the issue with moving the scroll bar when earlier content is changed, it seems like all that's needed is to make sure the call to SetTopLine at Editor.cxx:2679 always happens (Although this is also skipped when the MultiStepUndoRedo flag is set so I'm not sure how it works there). My understanding is that SetScrollBars and SetVerticalScrollPos can safely be called just once at the end of the batch since they don't do any relative adjustments to the scroll position

For your last suggestion, I've been working on the assumption that it's not necessary to make such low level changes, since the undo/redo optimisations already get away with not doing that. I'd certainly prefer to keep this as a more general 'suspend visual updates' optimisation if possible instead of getting into the specifics of how the scrollbars are modified. It's up to you if returning old data for scrollbar queries during batches is too much of a problem to get away with it though

I'll start on an updated patch with a new message type and the Scintilla.iface changes you mentioned below. Please let me know if I've missed anything important regarding this approach

[Neil Hodgson]

Although this is also skipped when the MultiStepUndoRedo flag is set so I'm not sure how it works there

Just not noticed before - undo groups are often over shorter ranges than the replace all of this issue.

Applications may want to wrap simple operations as batches then sequences of these in batches. Nested batches could use a counting batchModificationInProgress.

Naming this 'batch' may make users think there is more to this than just deferring scroll bar updates. It doesn't, for example, wrap the operations in an undo group or defer/coalesce notifications to the application. A name that mentions 'scroll bar' (or 'visual changes') may be better. It also defers some Redraw calls but I don't think all. It only affects text insertion and deletion and not, for example, adding annotation lines. The current scope is probably fine but it should be made clear in the documentation what the limitations are.

[Nick Powell]

I agree that allowing nested batches is a good idea

My concern with making the name too specific (e.g. 'suspend/resume scrollbar updates' or similar) is that this flag is stored and sent out by the Document class, and it doesn't make much sense to me for the internal document representation to care about GUI changes. Also I expect there are other places where this flag could allow other types of optimisations in future, and in that situation you'd have to break the API if you wanted to rename it to something more general

My suggestion is to call it "HintBatchModificationStarted" or something similar. I think that makes it pretty clear that it doesn't really do anything by itself but it may help performance in some situations

[Neil Hodgson]

Since it is only affecting visual updates then maybe it should be on the view instead of the document. NotifyModified can look at its local batchModificationInProgress (or scrollBarFrozen) member when deciding whether to perform scroll bar updates.

The Win32 WM_SETREDRAW message is meant for this sort of thing. Turning off redraw on Scintilla using WM_SETREDRAW speeds up the 'doubling \n' example by a factor better than 4 for me. Much of the remaining cost is still in SetScrollInfo so there could be some optimization performed to avoid calling it while in non-redraw state.
https://devblogs.microsoft.com/oldnewthing/20140407-00/?p=1313
https://devblogs.microsoft.com/oldnewthing/20110124-00/?p=11683
https://docs.microsoft.com/en-us/windows/win32/gdi/wm-setredraw

[Zufu Liu]

https://docs.microsoft.com/en-us/windows/win32/gdi/wm-setredraw says
WS_VISIBLE is removed when WM_SETREDRAW is called with wParam=FALSE, maybe following change would helps:

int ScintillaWin::SetScrollInfo(int nBar, LPCSCROLLINFO lpsi, BOOL bRedraw) noexcept {
    if (GetWindowStyle(MainHWND()) & WS_VISIBLE) {      
        return ::SetScrollInfo(MainHWND(), nBar, lpsi, bRedraw);
    }
    return 0;
}

it seems better to suppress all scrollbar updating when WS_VISIBLE is not set, and update once when WS_VISIBLE is set again.

[Neil Hodgson]

That speeds it up by a factor of 27 over the original. After the change a profile showed GetScrollInfo as a cost and removing that by quick exit in ChangeScrollPos and ModifyScrollBars halved the time again.

As well as SetScrollBars, updating afterwards requires calls to SetVerticalScrollPos and SetHorizontalScrollPos as, for stability, SetScrollBars only updates the vertical scroll position if it is past its maximum value.

[Nick Powell]

Sorry, I didn't have any time to look at this yesterday. So what are you thinking the API should look like with this approach? The user explicitly calls something like SetRedraw(false) on each editor connected to the document?

Also is there anything I can help with? It seems like you have the change pretty much completely written already

[Neil Hodgson]

The application would have to send WM_SETREDRAW to each Scintilla instance that it wants to stop updating.

WM_SETREDRAW stops more updating than the current BeginBatchModification patch including adding and removing annotation lines. BeginBatchModification is cross-platform although its on Windows that scroll bar updates are noticeably slow.

There doesn't have to be a Scintilla API as WM_SETREDRAW is a Win32 API. The equivalent on GTK is gdk_window_freeze_updates but I haven't tried that.

The application developer has to work out which operations need this then insert code to stop / start updates. There's an extra redraw even when nothing changed, such as when there are no matches for Replace All.

It may be possible for Scintilla to detect that it is inside a sequence of modifications with no changes visible on screen (no WM_PAINT messages received, unsure if updates to scroll bars can be detected) then buffer scroll bar changes and redrawing until the end of the sequence. Then applications wouldn't have to work out which actions may be slow and add code. Automatic update buffering is significantly more complex so its OK for WM_SETREDRAW to be added now.

[Neil Hodgson]

The API is defined in include/Scintilla.iface. scripts/ScintillaAPIFacer.py then generates the other files from Scintilla.iface. So Scintilla.iface should change similar to:

--- a/include/Scintilla.iface
+++ b/include/Scintilla.iface
@@ -3037,6 +3037,12 @@
 # Retrieve the position measured in index units at the start of a document line.
 fun position IndexPositionFromLine=2714(line line, LineCharacterIndexType lineCharacterIndex)

+# Begin a batch of modifications that should not update the scroll bar
+fun void BeginBatchModification=2736(,)
+
+# End a batch of modifications and update the scroll bar
+fun void EndBatchModification=2737(,)
+
 # Start notifying the container of all key presses and commands.
 fun void StartRecord=3001(,)

@@ -3179,7 +3185,8 @@
 val SC_MOD_INSERTCHECK=0x100000
 val SC_MOD_CHANGETABSTOPS=0x200000
 val SC_MOD_CHANGEEOLANNOTATION=0x400000
-val SC_MODEVENTMASKALL=0x7FFFFF
+val SC_MOD_IN_BATCH=0x800000
+val SC_MODEVENTMASKALL=0xFFFFFF

 ali SC_MOD_INSERTTEXT=INSERT_TEXT
 ali SC_MOD_DELETETEXT=DELETE_TEXT

[Neil Hodgson]

Committed as [e49f7d].

[Zufu Liu]

In SciTE, SetRedraw() can be used in SciTEBase::FoldAll() to make toggle folds faster.

[Neil Hodgson]

Committed as [e49f7d].