WIP:Disable highlighting after 512 characters on a line.
ClosedPublic

Authored by cullmann on Nov 29 2018, 7:56 PM.

Details

Summary

A lot of time is spent in decorationsForLine() when we have long lines.

Disabling highlighting after 512 characters improves the rendering speed very much, but again it is not so nice to not have highlighting.

Nice test case, see https://bugs.kde.org/show_bug.cgi?id=345775

Test Plan

Edit a text file with 100000 characters on a line and test that it still is possible to edit the file without unacceptable freezes.

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sars created this revision.Nov 29 2018, 7:56 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptNov 29 2018, 7:56 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
sars requested review of this revision.Nov 29 2018, 7:56 PM
brauch added a subscriber: brauch.Nov 29 2018, 8:04 PM

Shouldn't this change simultaneously remove the line length limit ...?

sars retitled this revision from Disable highlighting for lines longer than 1024 characters. to WIP: Disable highlighting for lines longer than 1024 characters..Nov 29 2018, 8:06 PM
sars edited the summary of this revision. (Show Details)
sars added a comment.Nov 29 2018, 8:29 PM

If we get it working properly we could make the limit much higher. I tried with 30000 characters and it still worked very good. (except when selecting the line)

sars updated this revision to Diff 46632.Dec 1 2018, 5:23 PM

Also disable highlighting in selections for lines that are longe than 1024

sars updated this revision to Diff 46633.Dec 1 2018, 5:41 PM
sars retitled this revision from WIP: Disable highlighting for lines longer than 1024 characters. to Disable highlighting for lines longer than 1024 characters..
sars edited the summary of this revision. (Show Details)
sars added a reviewer: dhaumann.

(Remove the unrelated change in src/view/kateview.cpp)

This version now disables the highlighting for lines longer than 1024 characters also when the line is selected. The selection highlight is shown tho.

What is the way forward? Do we apply this? Should we modify the line length limit?

I have now tested with a file with 97000 columns and I can start to see some slowdown but it is still usable.

I think we could raise the limit to 50000, but also add a notification if the file contains lines longer than 1024.

mwolff requested changes to this revision.Dec 4 2018, 9:33 AM
mwolff added a subscriber: mwolff.

what sven said, we should also remove the code to disable highlighting altogether when the line limit is reached, no?

src/render/katerenderer.cpp
399

put the 1024 into a constant and use it here and below such that we ensure the number stays in sync

also, don't we have a setting for the line length limit? shouldn't that be used instead here?

414

this style-change should be submitted independently of this code review

This revision now requires changes to proceed.Dec 4 2018, 9:33 AM
sars added a comment.Dec 4 2018, 11:30 AM

I agree, we need to do something about the line length limit (that only wraps the lines at the limit when opening the file).

The question is: do we totally remove the option and limit or only the option and raise the limit.

You can still edit the document at 200 000 characters, but it starts to get sluggish. At 500 000 it takes almost a second to insert a space (on my computer). If we keep the limit, we probably could be one of the few editor that could open and read files with millions of characters on a line.

If we keep the limit what would it be? 100000, 65536, 50000 or what?

Or do we keep the option and just raise the default limit?

I'm currently working on adding a notice message about the disabled highlighting.

There also seems to be a test for the long lines that needs to be updated.

sars retitled this revision from Disable highlighting for lines longer than 1024 characters. to WIP:Disable highlighting for lines longer than 1024 characters..Dec 4 2018, 11:30 AM
sars updated this revision to Diff 46957.Dec 6 2018, 1:44 PM
sars edited the summary of this revision. (Show Details)

Add a message to inform about why the lines are not highlighted.
Add a note about disabled highlighting to the wrapped lines warning.
Increase the default line length limit to 100 000 characters per line.

sars added a comment.Dec 6 2018, 1:55 PM

The highlighting limit is now returned in a function in KateRenderer as it is used also in katedocument.cpp for the warning/information message.

src/render/katerenderer.cpp
414

I figured that since I'm indenting the whole section and touching the line anyways, that fixing the style does not hurt. ;)

(Phabricator is not showing the white-space change)

dhaumann added a comment.EditedDec 8 2018, 4:39 PM

Is it correct that highlighting in the document (i.e. KSyntaxHighlighting) still takes place for the entire line, and this change simply only changes the fact that KateRenderer stops using the highlighting info after 100000 columns? And the next line is highlighted normally again, right?

Prior to accepting this patch, would it be possible to fix this differently, i.e. by profiling and fixing this properly instead of applying a workaround?

src/buffer/katetextbuffer.cpp
71 ↗(On Diff #46957)

Hm, isn't the line length limit configurable in kateconfig.cpp? There you will find a key called

const char KEY_LINE_LENGTH_LIMIT[] = "Line Length Limit";

that is used to read/write the config file.

I think if this is changed here to 100000, it should be changed to the same value in KateDocumentConfig::readValue():

setLineLengthLimit(config.readEntry(KEY_LINE_LENGTH_LIMIT, 4096));

And I would even suggest to rename as follows:

  • const char KEY_LINE_LENGTH_LIMIT[] = "Line Length Limit"; + const char KEY_LINE_LENGTH_LIMIT[] = "Line Length Limitation";

Since then, all users will automatically get a reset here. Without this change, all users will stay on the 4096 limit.

Guys this is great work. Can we please make the limits configurable? Each developer will have a different sweet spot. I don't want to burden this revision, maybe we can make a bug about configuring the limits and leave that part to someone confortable with the configs.

sars updated this revision to Diff 47146.Dec 8 2018, 8:24 PM
  • Use a bit different config key to force a value reset for the limit.
  • Fix reading the config value.
  • Update the line-highlight disabled info message and position.
sars marked 2 inline comments as done.Dec 8 2018, 8:53 PM

@dhaumann: You are right, the highlighting of the document still takes place and it is only KateRenderer that stops using the highlighting info for all lines longer than 1024 characters. All shorter lines are highlighted.

So the length limit option is still the same: Wrapping long lines on opening a document, just 25 times longer ;)

The disabled high-lighting is a workaround, but if we don't bring in some radical changes it will choke at some point anyways. So I would say that the target is to get the limits as high as possible.

I saw that Visualstudio code uses a similar limit for highlighting lines, but I don't know if that disables the highlighting totally....

I found this workaround by running Kate in Hotspot ;) I can continue a bit to see if I can get the 1024 limit raised...

sars added a comment.Dec 8 2018, 8:59 PM

@zetazeta I'm not sure the highlighting length limit is worth an option as the editing of the document is not effected. I it is just a bit inconvenient that the highlighting disappears...

sars updated this revision to Diff 48461.Dec 31 2018, 6:22 PM

Disable highlighting after [limit] characters on a line in stead of the whole line.

sars retitled this revision from WIP:Disable highlighting for lines longer than 1024 characters. to WIP:Disable highlighting after 512 characters on a line..Dec 31 2018, 6:26 PM
sars edited the summary of this revision. (Show Details)
sars edited the test plan for this revision. (Show Details)
sars marked an inline comment as done and an inline comment as not done.Dec 31 2018, 6:39 PM

The new limit is now limiting the number of characters highlighted on a line. It is only the rest of the line that is not highlighted. This is IMO nicer as you don't see the non-highlighted part at the beginning of the line. The highlighting makes it a bit slower, so I decreased the limit to 512.

I have also made the limit into a parameter so that it can be only 100 characters for the minimap highlighting.

I'm not convinced this is a good idea: Kile users writing LaTeX tend to write one paragraph into a single line. They easily hit the 512 character limit. With this patch, we'd break syntax highlighting (completely?) for Kile.

Stupidly asking: Can't this be opzimized evrn further?

sars added a comment.Jan 3 2019, 7:44 AM

@dhaumann OK the limit is too low for Kile that is clear. Visual Studio Code is limiting the highlighting on a line to 10000 characters.

I tried to set the limit to 10000, but that was very noticeably slow. Selecting a whole line took multiple seconds, which is probably the reason why we have had 4096 as the wrap limit ;)

Is it the whole idea of limiting the highlights or just the too low limit that you object to?

The main hotspots I see in my perf/hotspot profiling is RenderRangeList::advanceTo(...) in KateRenderer::decorationsForLine() and in KateRenderer::paintTextLine() the hotspot is QTextLayout::draw() (especially the one with "additionalFormats").

In both places I don't see (right now at least) very many possibilities to optimize.

I think the main problem is that we draw the whole line at once even tho we only see just a tiny bit of it (when we have long lines).

"Fixing" this problem would probably require that we also start to draw the lines in chunks...

Sorry to chime in that late ;=)

Wouldn't it make more sense, to limit the number of ranges per line?
I think the real issue is that we can't handle XXXX highlightings per line, not that the raw number of letters is that much an issue.

This would relax the constraint for most use cases.

cullmann requested changes to this revision.Mar 4 2019, 7:08 PM

I think we shall count the number of ranges on the line to judge the limit, otherwise this is a sound idea.

This revision now requires changes to proceed.Mar 4 2019, 7:08 PM
cullmann commandeered this revision.Mon, Jun 10, 6:45 PM
cullmann edited reviewers, added: sars; removed: cullmann.

I have drafted a small limit to attributes like I proposed.

cullmann updated this revision to Diff 59548.Mon, Jun 10, 6:45 PM
cullmann marked an inline comment as done.

Here is my try on this.
Instead of limiting the line length, we just limit the number of attributes we are allowed to display.
That only hits stuff != selection.
e.g. for the example out of https://bugs.kde.org/show_bug.cgi?id=345775 you still see selection but not other stuff.

cullmann edited the summary of this revision. (Show Details)Mon, Jun 10, 6:48 PM
cullmann updated this revision to Diff 59550.Mon, Jun 10, 7:15 PM

Remove a MASS of heap allocations.
We allocated the range on the heap and then inside we really even allocated all 16 byte KTextEditor::Range objects again on the heap.

Could this be pushed even further by using QVarLengthArray?

For the outer vector, yes.

sars added a comment.Sat, Jun 15, 6:59 PM

I tried the patch and it improved the performance really much! :) I was able to edit a line that contained over a million characters!

Unfortunately I can't comment on the code too much, as I don't know the code enough.

Some general stuff:

  1. Should there be a warning/information-message about why the highlighting is disabled?
  2. Should the line length limit be increased?

Great work!

I am not sure about the warning.
The line length limit default should be increased it think, we could just try something like 10000 for a start.
I would propose we try this without a warning and see if we get negative feedback.
Normally this should only kick in situations were before we slowed down to "unusable".
Would that be ok?

Then let's try that. Even for all cases without hitting the limit the new code is faster.

This revision was not accepted when it landed; it landed in state Needs Review.Sun, Jun 16, 9:11 AM
This revision was automatically updated to reflect the committed changes.

4daee27406cad0001f30bca4bf551495e0624d23

>

  • addConfigEntry(ConfigEntry(LineLengthLimit, "Line Length Limit", QString(), 4096));

+ addConfigEntry(ConfigEntry(LineLengthLimit, "Line Length Limit", QString(), 10000));

mwolff added a comment.EditedMon, Jun 17, 9:57 PM
This comment has been deleted.
src/render/katerenderer.cpp
408

couldn't the al.count be limited here? if I understand the patch correctly, we throw out all highlighting if we encounter too many ranges. but could we instead just skip the items that are beyond the limit?

Hmm, would a partially highlighted line really preferable?

sars added a comment.Tue, Jun 18, 10:20 AM

I think a partially highlighted line is better than a totally non-highlighted one. And I think that the user is more likely to instinctively guess correctly why the end of the line is not highlighted than if the line is not highlighted at all.