#2339 Folding: Scintilla 5 vs. Scintilla 4

[Yaron]

Using Notepad++.

https://github.com/notepad-plus-plus/notepad-plus-plus/commit/3b0479309729cbcdd29fa2a458b9b3e842543312

https://github.com/notepad-plus-plus/notepad-plus-plus/issues/11814

Can anyone think of the possible cause to the major performance difference between Scintilla 5 and Scintilla 4?

Thank you.

[Neil Hodgson]

The line in question doesn't appear to be directly calling Scintilla so its difficult to know what to examine.

_scintView.collapse(searchHeaderLevel - SC_FOLDLEVELBASE, fold_collapse);

You should produce a profile showing in which functions the time is spent.

There has been an optimization to folding with [4f6cd7] that avoids styling text. Its possible that this causes unexpected behaviour. You could revert to before this change to discover if it is the problem.

More generally, try to find the change set where the issue starts occurring, perhaps by using hg bisect.

[Yaron]

@nyamatongwe,

Thank you for looking into it and replying.

Commit: [4f6cd7] seems to be related.

I've asked Don to join this discussion.
https://github.com/notepad-plus-plus/notepad-plus-plus/issues/11814#issuecomment-1172779922

[Neil Hodgson]

Commit: [4f6cd7] seems to be related.

That likely means its performing multiple small lexings instead of one big one. The trade-offs between different ways of lexing the document can be complex with different strategies performing better in different circumstances. There could be a call to SCI_COLOURISE(SCI_GETENDSTYLED(), -1) before folding.

[Don HO]

Hi Neil

Thank you for looking into it.
My old implementation is surely not optimised.
But even I use
_scintView.execute(SCI_TOGGLEFOLD, 0);
instead of
_scintView.collapse(searchHeaderLevel - SC_FOLDLEVELBASE, fold_collapse);
The result in release delay about 1-3 seconds, in debug mode is about 6-9 seconds (on a gamer PC)

[Neil Hodgson]

I'm not really seeing a good reason for a performance problem here. A profile may reveal the root cause. Scroll bar changes can slow down bulk operations (the referenced issue doesn't say how many lines are involved) and this can sometimes be improved since Scintilla 5.1.1 by turning off redraw (WM_SETREDRAW) for the operation.

[Yaron]

the referenced issue doesn't say how many lines are involved.

The exact Steps to Reproduce the Issue are in https://github.com/notepad-plus-plus/notepad-plus-plus/issues/11814#issue-1275338700

I hope Don would look into it and continue the discussion.

Thanks again for your helpful replies.

[Zufu Liu]

Test toggle all folds speed for following 6MB html file (with syntax highlighting) : SciTE > Notepad2 > notepad++, and notepad++ 8.4.2 become unresponsive on my system.
https://github.com/whatwg/html/blob/main/source (download and save as html).

[Neil Hodgson]

There are various possibilities involving lexing the lines in the buffer in a way that is costly, particularly if the lexer isn't optimized for incremental use. If a lexer seeks back to the beginning of the buffer or to a header before doing its work then it is going to see poor performance if it is called repeatedly.

A profile can answer questions about what is occurring: maybe its the scroll bar, maybe its repeated redraws, maybe its lexing.

I recall Notepad++ has a second layer of folding for hiding lines. Perhaps this is interacting poorly with some changes made in Scintilla 5.

[Zufu Liu]

it turns out the performance bottleneck is Document::GetLastChild() and ContractionState<LINE>::SetVisible().

After the document is full styled, comment out EnsureStyledTo(LineStart(lineMaxSubord + 2)); makes fold all levels 2x faster on my system, attached the change for Document::GetLastChild(), changed the code to style on demand to avoid unneeded call to LineStart().

if (lineMaxSubord >= lineEndStyled) {
    EnsureStyledTo(LineStart(lineMaxSubord + 2));
    lineEndStyled = SciLineFromPosition(GetEndStyled());
}

[Neil Hodgson]

LineStart is low cost compared to lexing and is unlikely to be significant - lexers often call several similar complexity methods for each line.

The GetLastChild.cxx change performs lexing in batches of 2 lines instead of line by line. The SciLineFromPosition can be removed - although this can end up with lineEndStyled past the end.

lineEndStyled = lineMaxSubord + 2;
EnsureStyledTo(LineStart(lineEndStyled));

Thus the lexer initialization and finalization costs are halved and are likely dominating. If lexing 2 lines at at time is better then some greater factor may be worthwhile but there will be a trade-off here when the extra lines do not need to be lexed.

[Zufu Liu]

Something used by idle styling? e.g. LineFromPositionAfter() 2 or more lines estimated with a small duration?

[Zufu Liu]

SciLineFromPosition is needed for Notepad++'s buggy LexSearchResult.cxx , under some condition it does nothing (no update to endStyled).

https://github.com/notepad-plus-plus/notepad-plus-plus/blob/master/lexilla/lexers/LexSearchResult.cxx#L116

[Neil Hodgson]

Looks like it needs a Flush().

[Neil Hodgson]

LexSearchResult should be fixed - this shouldn't be covered up by Scintilla.

[Zufu Liu]

I think this change is enough, my original purpose is to prevent exponential call toEnsureStyledTo(LineStart(lineMaxSubord + 2)) even if the document is whole styled. e.g. application like Notepad++ or old Notepad2 that uses SCI_TOGGLEFOLD or SCI_FOLDLINE to implement custom folding (especially fold all, which calls Editor::FoldLine() on every header line).

[Neil Hodgson]

This is also changing the behaviour to style in 2 line blocks which isn't obvious. I'm concerned that lines are being treated unevenly with different lines having different amounts of styling performed. Is fold data discovered as accurately as before or could there be issues with some folders?

[Zufu Liu]

EnsureStyledTo(LineStart(lineMaxSubord + 2)); is to ensure line lineMaxSubord + 1, aka next line is styled.

This does not change the behavior, it just avoided the call when next line is already styled.
it's lineMaxSubord + 1 > lineEndStyled in the GetLastChild.cpp file, converted to lineMaxSubord >= lineEndStyled to avoid extra addition.

[Neil Hodgson]

The EnsureStyledTo does the same work as before when called but is only called for half the lines.

[Zufu Liu]

The code is correct, no line will be skipped

if (lineMaxSubord >= lineEndStyled) {
    EnsureStyledTo(LineStart(lineMaxSubord + 2));
    lineEndStyled = SciLineFromPosition(GetEndStyled());
}

as EnsureStyledTo() is doing

const Sci::Line lineEndStyled = SciLineFromPosition(GetEndStyled());
const Sci::Position endStyledTo = LineStart(lineEndStyled);
pli->Colourise(endStyledTo, pos);

[Neil Hodgson]

I didn't respond before since the unedited comment which I saw in an email was different.

While no line will (eventually) be skipped, the point at which the level is read may see different data.

Using this 10-byte text "{\r\n\r\n\r\n}\r\n":

{


}

Placing tracepoints:
In EnsureStyledTo as Styling to {pos}
On instance->Lex in LexInterface::Colouriseas Lex {start} {len}
In GetFoldLevel as GetFoldLevel {line}.

Forcing endStyled back to 0 before starting.

Running current release:

Styling to 5
Lex 0 5
GetFoldLevel 1
Styling to 7
Lex 5 2
GetFoldLevel 2
Styling to 10
Lex 7 3
GetFoldLevel 3
GetFoldLevel 4
GetFoldLevel 5

Running patched code:

Styling to 5
Lex 0 5
GetFoldLevel 1
GetFoldLevel 2
Styling to 10
Lex 5 5
GetFoldLevel 3
GetFoldLevel 4
GetFoldLevel 5

So, the patched code is checking line 2 when 5 bytes (2 lines "{\r\n\r\n") have been lexed but the original code had 7 bytes lexed (3 lines "{\r\n\r\n\r\n").

[Zufu Liu]

Seems to me the patch avoid a lex for line 2 as the whole line (first 5 bytes) already lexed?

I just realized two or more lines need to be styled in EnsureStyledTo() as some lexer (e.g. LexDiff) may change fold level for previous line.

[Zufu Liu]

Oh, I'm wrong, on GetFoldLevel 2, line 3 is not yet lexed.

Sci::Line lineEndStyled = SciLineFromPosition(GetEndStyled()) - 1;
Sci::Line lineMaxSubord = lineParent;
while (lineMaxSubord < maxLine) {
    if (lineMaxSubord >= lineEndStyled) {
        EnsureStyledTo(LineStart(lineMaxSubord + 2));
        lineEndStyled = SciLineFromPosition(GetEndStyled()) - 1;
    }

[Zufu Liu]

The performance for SetVisible() can be solved by avoid repeated hiding children lines when current header line is already hided (hided by folding it's parent level) with following changes in Editor::FoldLine() and Editor::FoldAll():

if (pcs->GetVisible(line)) {
    pcs->SetVisible(line + 1, lineMaxSubord, false);
}

[Neil Hodgson]

I'd leave this to the Notepad++ developers to see if it makes a significant difference. It appears to me that it will only have a minor effect.