-
Notifications
You must be signed in to change notification settings - Fork 236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
highlighter: autostop co-routine when not needed #881
Conversation
to be more specific this issues can be noticed on mardown files that have long lines like the readme on lite-xl-plugins which have the table with really long lines. Maybe something needs to be yielded when tokenizing a single line or something related to sub syntaxes. |
This change improves the responsiveness of this readme but there is something really wrong with long lines since even loading the file and saving takes some time.... diff --git a/data/core/doc/highlighter.lua b/data/core/doc/highlighter.lua
index c8d9d47..2ca5806 100644
--- a/data/core/doc/highlighter.lua
+++ b/data/core/doc/highlighter.lua
@@ -5,6 +5,9 @@ local tokenizer = require "core.tokenizer"
local Object = require "core.object"
+local long_line = 255
+
+
local Highlighter = Object:extend()
@@ -18,13 +21,12 @@ function Highlighter:new(doc)
end
---- init incremental syntax highlighting
function Highlighter:start()
if self.running then return end
self.running = true
core.add_thread(function()
while not self.exit do
-
+
-- for some strange reason some docs are created at startup without any
-- name so we should also exit if doc has no name and not in core.docs
if not self.doc.filename then
@@ -52,6 +54,10 @@ function Highlighter:start()
if not (line and line.init_state == state and line.text == self.doc.lines[i]) then
self.lines[i] = self:tokenize_line(i, state)
end
+ if #self.doc.lines[i] > long_line then
+ core.redraw = true
+ coroutine.yield()
+ end
end
self.first_invalid_line = max + 1
@@ -116,9 +122,18 @@ end
function Highlighter:get_line(idx)
local line = self.lines[idx]
if not line or line.text ~= self.doc.lines[idx] then
- local prev = self.lines[idx - 1]
- line = self:tokenize_line(idx, prev and prev.state)
- self.lines[idx] = line
+ if #self.doc.lines[idx] > long_line then
+ line = {
+ tokens = {
+ "normal",
+ self.doc.lines[idx]
+ }
+ }
+ else
+ local prev = self.lines[idx - 1]
+ line = self:tokenize_line(idx, prev and prev.state)
+ self.lines[idx] = line
+ end
end
self.max_wanted_line = math.max(self.max_wanted_line, idx)
return line |
Note: After some profiling one of the performance degraders is detectindent which is also using the tokenizer at startup with the same syntax of the document which seems too much patterns for what we want to check, this should be simplified with a more basic and performant solution. |
As an update to this issue, the problem was really apparent on luajit. But, after some discussion with Guldoman the circular weak ref used by the co-routine on Highlighter is properly collected by lua non jit version when the document is closed after a couple of seconds, but some plugins like lint+ that may have loose references to the doc will prevent garbage collection and leave the co-routine running until the document is re-opened which then looses the old references (maybe not) and creates new ones. So with lua5.4 the major issue is plugins that not properly release references to a loaded doc, which would mean that opening documents and closing will leave these co-routines running. This could be negligible when the amount of plugins that could introduce such an issue is small otherwise it turns into a witch hunt when trying to fix the plugin culprit. |
May I ask you to adapt this fix for master-2.0 branch ? I may publish a 2.0.6 release with some minor fixes. |
Sorry that I haven't done anything on this, I did fixed the lint+ plugin that was causing the co-routine leakage, so at least when using lua 5.4 it should not happen at least with that plugin which so far seems to be the only one that presented the issue on non jit lua. Still, this fix would benefit the luajit target (if releasing luajit builds is still of interest) which doesn't properly handles weak references, but I wanted to first get #885 in and then rebase this PR in top of it and maybe have a better discussion about it, but surely this change would benefit the luajit branch. |
Thank you for your answer. Not everything is completely clear to me, I didn't know that luajit had problems with weak references. I may do some investigations to see if for the master-2.0 branch with luajit there is a real coroutine leak. |
Let me know how your testing goes |
d877acc
to
dc4fd38
Compare
dc4fd38
to
12d631d
Compare
2b5d063
to
4fe2173
Compare
cf04ac4
to
a653380
Compare
681a8ba
to
0ef370b
Compare
Turns out we really need this since there isn't an elegant way to stop the highlighter coroutine on elements that inherit from the When applying on widgets |
67d258f
to
d8d81aa
Compare
After some discussion with @Guldoman he came with the idea of auto stopping the highlighter co-routine automatically when not needed which is actually a better solution on the resources/performance department since now having a bunch of documents open means that unnecessary highlighting co-routines will not be running on the background. So this solution should be an overall win, |
cf520f8
to
d339acd
Compare
* highlighter: autostop co-routine when not needed * applied @Guldoman suggestions
The highlighter creates a coroutine for incremental tokenization, but each time a document is reset, re-opened, a command view executed, etc... a new co-routine is added and the previous one never stopped. Found this issue while changing the rainbowparen.lua plugin, here a video with print debugging 😄, notice that at startup three co-routines for highlight are created for docs that don't even have name, some one else more familiarized with the related code should investigate it, also each time you close and open a file or commandview a new co-routine is created which isn't nice for the cpu usage:
co-leak.mp4
Don't know if this PR is the best solution, I don't see much gain on that incremental highlighter, maybe it is doing yield incorrectly since the highlighting on files which syntax has lots of patterns is taking too much to load and scroll, should be performed more lazily (inside the for loop instead of outside). Also disabling the incremental highlighter makes performance worse than original lite.