bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Juri Linkov]

X-Debbugs-Cc: Dmitry Gutov <dgutov@yandex.ru>

After more rules were added recently to ruby-ts--font-lock-settings,
font-lock became slow even on very small files.  Some measurements:

M-: (benchmark-run 1000 (progn (font-lock-mode -1) (font-lock-mode 1) (font-lock-ensure)))

M-x ruby-mode
(1.3564674989999999 0 0.0)

M-x ruby-ts-mode
(8.349582391999999 2 6.489918534000001)

This is not a problem when files are visited infrequently, but
becomes a problem for diff-syntax fontification that wants to
highlight simultaneously many files from git logs.
So a temporary measure would be not to enable ruby-ts-mode
in internal buffers:

(add-hook 'find-file-hook
          (lambda ()
            (when (and (eq major-mode 'ruby-mode)
                       ;; Only when not internal as from diff-syntax
                       (not (string-prefix-p " " (buffer-name))))
              (ruby-ts-mode))))

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Dmitry Gutov]

Hi!

On 09/01/2023 19:16, Juri Linkov wrote:
> X-Debbugs-Cc: Dmitry Gutov <dgutov@yandex.ru>
> 
> After more rules were added recently to ruby-ts--font-lock-settings,
> font-lock became slow even on very small files.  Some measurements:

If you saw a particular commit that made things slower, did you try 
reverting it? What was the performance after?

> M-: (benchmark-run 1000 (progn (font-lock-mode -1) (font-lock-mode 1) (font-lock-ensure)))
> 
> M-x ruby-mode
> (1.3564674989999999 0 0.0)
> 
> M-x ruby-ts-mode
> (8.349582391999999 2 6.489918534000001)

I have tried this scenario (which, to be frank, is pretty artificial, 
given that fontification is usually performed in chunks, not over the 
whole buffer).

Perhaps the results depend on a particular file. The ones I have tried 
(ruby.rb and ruby-after-operator-indent.rb) show only 2x difference (or 
less). The difference was in favor of ruby-mode, but given the 
difference in approaches I wouldn't be surprised if ruby-ts-mode incurs 
a fixed overhead somewhere.

> This is not a problem when files are visited infrequently, but
> becomes a problem for diff-syntax fontification that wants to
> highlight simultaneously many files from git logs.
> So a temporary measure would be not to enable ruby-ts-mode
> in internal buffers:

Is it common to try to highlight 1000 or even 100 files in one diff?

> (add-hook 'find-file-hook
>            (lambda ()
>              (when (and (eq major-mode 'ruby-mode)
>                         ;; Only when not internal as from diff-syntax
>                         (not (string-prefix-p " " (buffer-name))))
>                (ruby-ts-mode))))

Have you tried similar tests with other -ts- modes? Ones with complex 
font-lock rules in particular.

I've tried commenting out different rules in 
ruby-ts--font-lock-settings, but none of them seem to have particularly 
outsides impact. Performance seems, roughly, inversely proportional to 
the number of separate "features".

And if all ts modes turn out to have this problem, perhaps the place to 
improve this is inside some common code.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Juri Linkov]

>> After more rules were added recently to ruby-ts--font-lock-settings,
>> font-lock became slow even on very small files.  Some measurements:
>
> If you saw a particular commit that made things slower, did you try
> reverting it? What was the performance after?

No particular commit, just adding more rules degrades performance
gradually.

>> M-: (benchmark-run 1000 (progn (font-lock-mode -1) (font-lock-mode 1) (font-lock-ensure)))
>> M-x ruby-mode
>> (1.3564674989999999 0 0.0)
>> M-x ruby-ts-mode
>> (8.349582391999999 2 6.489918534000001)
>
> I have tried this scenario (which, to be frank, is pretty artificial, given
> that fontification is usually performed in chunks, not over the whole
> buffer).
>
> Perhaps the results depend on a particular file. The ones I have tried
> (ruby.rb and ruby-after-operator-indent.rb) show only 2x difference (or
> less). The difference was in favor of ruby-mode, but given the difference
> in approaches I wouldn't be surprised if ruby-ts-mode incurs a fixed
> overhead somewhere.

On test/lisp/progmodes/ruby-mode-resources/ruby.rb I see these numbers:

ruby-mode
(8.701560543000001 95 1.045961102)

ruby-ts-mode
(34.653148898000005 1464 16.904981779)

>> This is not a problem when files are visited infrequently, but
>> becomes a problem for diff-syntax fontification that wants to
>> highlight simultaneously many files from git logs.
>> So a temporary measure would be not to enable ruby-ts-mode
>> in internal buffers:
>
> Is it common to try to highlight 1000 or even 100 files in one diff?

100 is rare, but tens is pretty common, so this problem affects
only this specific case.

>> (add-hook 'find-file-hook
>>            (lambda ()
>>              (when (and (eq major-mode 'ruby-mode)
>>                         ;; Only when not internal as from diff-syntax
>>                         (not (string-prefix-p " " (buffer-name))))
>>                (ruby-ts-mode))))
>
> Have you tried similar tests with other -ts- modes? Ones with complex
> font-lock rules in particular.

I tried with c-ts-mode, and it's very fast.

> I've tried commenting out different rules in ruby-ts--font-lock-settings,
> but none of them seem to have particularly outsides impact. Performance
> seems, roughly, inversely proportional to the number of separate
> "features".

Indeed, this is what I see - no particular rule, only their number
affects performance.

> And if all ts modes turn out to have this problem, perhaps the place to
> improve this is inside some common code.

I noticed that while most library files are small, e.g.
libtree-sitter-c.so is 401,528 bytes,
libtree-sitter-ruby.so is 2,130,616 bytes
that means that it has more complex logic
that might explain its performance.

In this case, when nothing could be done to improve performance,
please close this request.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Dmitry Gutov]

On 10/01/2023 10:10, Juri Linkov wrote:
>>> After more rules were added recently to ruby-ts--font-lock-settings,
>>> font-lock became slow even on very small files.  Some measurements:
>>
>> If you saw a particular commit that made things slower, did you try
>> reverting it? What was the performance after?
> 
> No particular commit, just adding more rules degrades performance
> gradually.

But I don't think I added that many rules recently. No more than a 
quarter anyway.

>>> M-: (benchmark-run 1000 (progn (font-lock-mode -1) (font-lock-mode 1) (font-lock-ensure)))
>>> M-x ruby-mode
>>> (1.3564674989999999 0 0.0)
>>> M-x ruby-ts-mode
>>> (8.349582391999999 2 6.489918534000001)
>>
>> I have tried this scenario (which, to be frank, is pretty artificial, given
>> that fontification is usually performed in chunks, not over the whole
>> buffer).
>>
>> Perhaps the results depend on a particular file. The ones I have tried
>> (ruby.rb and ruby-after-operator-indent.rb) show only 2x difference (or
>> less). The difference was in favor of ruby-mode, but given the difference
>> in approaches I wouldn't be surprised if ruby-ts-mode incurs a fixed
>> overhead somewhere.
> 
> On test/lisp/progmodes/ruby-mode-resources/ruby.rb I see these numbers:
> 
> ruby-mode
> (8.701560543000001 95 1.045961102)
> 
> ruby-ts-mode
> (34.653148898000005 1464 16.904981779)

Interesting. It's 12s vs 36s for me, as I've retested now.

>>> This is not a problem when files are visited infrequently, but
>>> becomes a problem for diff-syntax fontification that wants to
>>> highlight simultaneously many files from git logs.
>>> So a temporary measure would be not to enable ruby-ts-mode
>>> in internal buffers:
>>
>> Is it common to try to highlight 1000 or even 100 files in one diff?
> 
> 100 is rare, but tens is pretty common, so this problem affects
> only this specific case.

So it's a 0,8-3s delay in those cases? That's not ideal.

>>> (add-hook 'find-file-hook
>>>             (lambda ()
>>>               (when (and (eq major-mode 'ruby-mode)
>>>                          ;; Only when not internal as from diff-syntax
>>>                          (not (string-prefix-p " " (buffer-name))))
>>>                 (ruby-ts-mode))))
>>
>> Have you tried similar tests with other -ts- modes? Ones with complex
>> font-lock rules in particular.
> 
> I tried with c-ts-mode, and it's very fast.

Just how fast is it? The number of font-lock features is has is 
comparable (though a little smaller).

I've tried the same benchmark for it in admin/alloc-colors.c, and it 
comes out to

   (3.2004193190000003 30 0.9609690980000067)

Which seems comparable.

Not sure how to directly test the modes against each other, but if I 
enable ruby-ts-mode in the same file, the benchmark comes to 1s.

Or if I enable c-ts-mode in ruby.rb -- 16s.

>> I've tried commenting out different rules in ruby-ts--font-lock-settings,
>> but none of them seem to have particularly outsides impact. Performance
>> seems, roughly, inversely proportional to the number of separate
>> "features".
> 
> Indeed, this is what I see - no particular rule, only their number
> affects performance.
> 
>> And if all ts modes turn out to have this problem, perhaps the place to
>> improve this is inside some common code.
> 
> I noticed that while most library files are small, e.g.
> libtree-sitter-c.so is 401,528 bytes,
> libtree-sitter-ruby.so is 2,130,616 bytes
> that means that it has more complex logic
> that might explain its performance.

ruby is indeed one of the larger ones. Among the ones I have here 
compiled, it's exceeded only by cpp. 2.29 MB vs 2.12 MB.

But testing admin/alloc-colors.c with c++-ts-mode vs c-ts-mode gives 
very similar performance, so it's unlikely that the complexity of the 
grammar is directly responsible.

> In this case, when nothing could be done to improve performance,
> please close this request.

Perhaps Yuan has some further ideas. There are some strong oddities here:

- Some time into debugging and repeating the benchmark again and again, 
I get the "Pure Lisp storage overflowed" message. Just once per Emacs 
session. It doesn't seem to change much, so it might be unimportant.

- The profiler output looks like this:

   18050  75%                    - font-lock-fontify-syntactically-region
   15686  65%                     - treesit-font-lock-fontify-region
    3738  15% 
treesit--children-covering-range-recurse
     188   0%                        treesit-fontify-with-override

- When running the benchmark for the first time in a buffer (such as 
ruby.rb), the variable treesit--font-lock-fast-mode is usually changed 
to t. In one Emacs session, after I changed it to nil and re-ran the 
benchmark, the variable stayed nil, and the benchmark ran much faster 
(like 10s vs 36s).

In the next session, after I restarted Emacs, that didn't happen: it 
always stayed at t, even if I reset it to nil between runs. But if I 
comment out the block in treesit-font-lock-fontify-region that uses it

     ;; (when treesit--font-lock-fast-mode
     ;;   (setq nodes (treesit--children-covering-range-recurse
     ;;                (car nodes) start end (* 4 jit-lock-chunk-size))))

and evaluate the defun, the benchmark runs much faster again: 11s.

(But then I brought it all back, and re-ran the tests, and the variable 
stayed nil that time around; to sum up: the way it's turned on is unstable.)

Should treesit--font-lock-fast-mode be locally bound inside that 
function, so that it's reset between chunks? Or maybe the condition for 
its enabling should be tweaked? E.g. I don't think there are any 
particularly large or deep nodes in ruby.rb's parse tree. It's a very 
shallow file.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Juri Linkov]

>>> Is it common to try to highlight 1000 or even 100 files in one diff?
>> 100 is rare, but tens is pretty common, so this problem affects
>> only this specific case.
>
> So it's a 0,8-3s delay in those cases? That's not ideal.

The delay is noticeable, alas.

>> I noticed that while most library files are small, e.g.
>> libtree-sitter-c.so is 401,528 bytes,
>> libtree-sitter-ruby.so is 2,130,616 bytes
>> that means that it has more complex logic
>> that might explain its performance.
>
> ruby is indeed one of the larger ones. Among the ones I have here compiled,
> it's exceeded only by cpp. 2.29 MB vs 2.12 MB.

The winner is libtree-sitter-julia.so with 7.25 MB.
But regarding libtree-sitter-cpp.so I confirm it's 2.3 MB.
And c++-ts-mode is even faster than c-ts-mode.
On the same admin/alloc-colors.c:

c-mode
(33.378821569 1500 17.632000617)

c-ts-mode
(2.1949608069999997 34 0.4119784769999981)

c++-ts-mode
(2.0979403910000003 34 0.39749122499999956)

So size doesn't matter.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Dmitry Gutov]

On 10/01/2023 19:50, Juri Linkov wrote:
>>>> Is it common to try to highlight 1000 or even 100 files in one diff?
>>> 100 is rare, but tens is pretty common, so this problem affects
>>> only this specific case.
>>
>> So it's a 0,8-3s delay in those cases? That's not ideal.
> 
> The delay is noticeable, alas.

Right. I'm somewhat worried for the processing speed 
xref--collect-matches too. But that's probably only going to be 
noticeable after we add syntax-propertize-function to ruby-ts-mode.

>>> I noticed that while most library files are small, e.g.
>>> libtree-sitter-c.so is 401,528 bytes,
>>> libtree-sitter-ruby.so is 2,130,616 bytes
>>> that means that it has more complex logic
>>> that might explain its performance.
>>
>> ruby is indeed one of the larger ones. Among the ones I have here compiled,
>> it's exceeded only by cpp. 2.29 MB vs 2.12 MB.
> 
> The winner is libtree-sitter-julia.so with 7.25 MB.
> But regarding libtree-sitter-cpp.so I confirm it's 2.3 MB.
> And c++-ts-mode is even faster than c-ts-mode.

Yep.

> On the same admin/alloc-colors.c:
> 
> c-mode
> (33.378821569 1500 17.632000617)
> 
> c-ts-mode
> (2.1949608069999997 34 0.4119784769999981)
> 
> c++-ts-mode
> (2.0979403910000003 34 0.39749122499999956)
> 
> So size doesn't matter.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Dmitry Gutov]

Yuan? Just making sure you got this message.

On 10/01/2023 16:10, Dmitry Gutov wrote:
> Perhaps Yuan has some further ideas. There are some strong oddities here:
> 
> - Some time into debugging and repeating the benchmark again and again, 
> I get the "Pure Lisp storage overflowed" message. Just once per Emacs 
> session. It doesn't seem to change much, so it might be unimportant.
> 
> - The profiler output looks like this:
> 
>    18050  75%                    - font-lock-fontify-syntactically-region
>    15686  65%                     - treesit-font-lock-fontify-region
>     3738  15% treesit--children-covering-range-recurse
>      188   0%                        treesit-fontify-with-override
> 
> - When running the benchmark for the first time in a buffer (such as 
> ruby.rb), the variable treesit--font-lock-fast-mode is usually changed 
> to t. In one Emacs session, after I changed it to nil and re-ran the 
> benchmark, the variable stayed nil, and the benchmark ran much faster 
> (like 10s vs 36s).
> 
> In the next session, after I restarted Emacs, that didn't happen: it 
> always stayed at t, even if I reset it to nil between runs. But if I 
> comment out the block in treesit-font-lock-fontify-region that uses it
> 
>      ;; (when treesit--font-lock-fast-mode
>      ;;   (setq nodes (treesit--children-covering-range-recurse
>      ;;                (car nodes) start end (* 4 jit-lock-chunk-size))))
> 
> and evaluate the defun, the benchmark runs much faster again: 11s.
> 
> (But then I brought it all back, and re-ran the tests, and the variable 
> stayed nil that time around; to sum up: the way it's turned on is 
> unstable.)
> 
> Should treesit--font-lock-fast-mode be locally bound inside that 
> function, so that it's reset between chunks? Or maybe the condition for 
> its enabling should be tweaked? E.g. I don't think there are any 
> particularly large or deep nodes in ruby.rb's parse tree. It's a very 
> shallow file.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Yuan Fu]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Yuan? Just making sure you got this message.

Sorry for the delay :-)

> On 10/01/2023 16:10, Dmitry Gutov wrote:
>> Perhaps Yuan has some further ideas. There are some strong oddities here:
>> - Some time into debugging and repeating the benchmark again and
>> again, I get the "Pure Lisp storage overflowed" message. Just once
>> per Emacs session. It doesn't seem to change much, so it might be
>> unimportant.

That sounds like 60653. The next time you encounter it, could you record
the output of M-x memory-usage and M-x memory-report? 

>> - The profiler output looks like this:
>>    18050  75%                    -
>> font-lock-fontify-syntactically-region
>>    15686  65%                     - treesit-font-lock-fontify-region
>>     3738  15% treesit--children-covering-range-recurse
>>      188   0%                        treesit-fontify-with-override
>> - When running the benchmark for the first time in a buffer (such as
>> ruby.rb), the variable treesit--font-lock-fast-mode is usually
>> changed to t. In one Emacs session, after I changed it to nil and
>> re-ran the benchmark, the variable stayed nil, and the benchmark ran
>> much faster (like 10s vs 36s).
>> In the next session, after I restarted Emacs, that didn't happen: it
>> always stayed at t, even if I reset it to nil between runs. But if I
>> comment out the block in treesit-font-lock-fontify-region that uses
>> it
>>      ;; (when treesit--font-lock-fast-mode
>>      ;;   (setq nodes (treesit--children-covering-range-recurse
>>      ;;                (car nodes) start end (* 4 jit-lock-chunk-size))))
>> and evaluate the defun, the benchmark runs much faster again: 11s.
>> (But then I brought it all back, and re-ran the tests, and the
>> variable stayed nil that time around; to sum up: the way it's turned
>> on is unstable.)
>> Should treesit--font-lock-fast-mode be locally bound inside that
>> function, so that it's reset between chunks? Or maybe the condition
>> for its enabling should be tweaked? E.g. I don't think there are any
>> particularly large or deep nodes in ruby.rb's parse tree. It's a
>> very shallow file.

Yeah that is a not-very-clever hack. I’ve got an idea: I can add a C
function that checks the maximum depth of a parse tree and the maximum
node span, and turn on the fast-mode if the depth is too large or a node
is too wide. And we do that check once before doing any fontification.

I’ll report back once I add it.

Yuan

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Dmitry Gutov]

On 12/01/2023 23:58, Yuan Fu wrote:
> 
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> Yuan? Just making sure you got this message.
> 
> Sorry for the delay :-)
> 
>> On 10/01/2023 16:10, Dmitry Gutov wrote:
>>> Perhaps Yuan has some further ideas. There are some strong oddities here:
>>> - Some time into debugging and repeating the benchmark again and
>>> again, I get the "Pure Lisp storage overflowed" message. Just once
>>> per Emacs session. It doesn't seem to change much, so it might be
>>> unimportant.
> 
> That sounds like 60653. The next time you encounter it, could you record
> the output of M-x memory-usage and M-x memory-report?

Managed to reproduce this after running the test in a couple of 
different files.

But 'M-x memory-usage' says no such command, and 'M-x memory-report' 
ends up with this error:

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
   memory-report--gc-elem(nil strings)
   memory-report--garbage-collect()
   memory-report()
   funcall-interactively(memory-report)
   #<subr call-interactively>(memory-report record nil)
   apply(#<subr call-interactively> memory-report (record nil))
   call-interactively@ido-cr+-record-current-command(#<subr 
call-interactively> memory-report record nil)
   apply(call-interactively@ido-cr+-record-current-command #<subr 
call-interactively> (memory-report record nil))
   call-interactively(memory-report record nil)
   command-execute(memory-report record)
   execute-extended-command(nil "memory-report" nil)
   funcall-interactively(execute-extended-command nil "memory-report" nil)
   #<subr call-interactively>(execute-extended-command nil nil)
   apply(#<subr call-interactively> execute-extended-command (nil nil))
   call-interactively@ido-cr+-record-current-command(#<subr 
call-interactively> execute-extended-command nil nil)
   apply(call-interactively@ido-cr+-record-current-command #<subr 
call-interactively> (execute-extended-command nil nil))
   call-interactively(execute-extended-command nil nil)
   command-execute(execute-extended-command)

garbage-collect's docstring says:

   However, if there was overflow in pure space, and Emacs was dumped
   using the "unexec" method, ‘garbage-collect’ returns nil, because
   real GC can’t be done.

I don't know if my Emacs was dumped using "unexec", though. ./configure 
says I'm using pdumper.

In case that matters, I'm testing the emacs-29 branch.

>>> - The profiler output looks like this:
>>>     18050  75%                    -
>>> font-lock-fontify-syntactically-region
>>>     15686  65%                     - treesit-font-lock-fontify-region
>>>      3738  15% treesit--children-covering-range-recurse
>>>       188   0%                        treesit-fontify-with-override
>>> - When running the benchmark for the first time in a buffer (such as
>>> ruby.rb), the variable treesit--font-lock-fast-mode is usually
>>> changed to t. In one Emacs session, after I changed it to nil and
>>> re-ran the benchmark, the variable stayed nil, and the benchmark ran
>>> much faster (like 10s vs 36s).
>>> In the next session, after I restarted Emacs, that didn't happen: it
>>> always stayed at t, even if I reset it to nil between runs. But if I
>>> comment out the block in treesit-font-lock-fontify-region that uses
>>> it
>>>       ;; (when treesit--font-lock-fast-mode
>>>       ;;   (setq nodes (treesit--children-covering-range-recurse
>>>       ;;                (car nodes) start end (* 4 jit-lock-chunk-size))))
>>> and evaluate the defun, the benchmark runs much faster again: 11s.
>>> (But then I brought it all back, and re-ran the tests, and the
>>> variable stayed nil that time around; to sum up: the way it's turned
>>> on is unstable.)
>>> Should treesit--font-lock-fast-mode be locally bound inside that
>>> function, so that it's reset between chunks? Or maybe the condition
>>> for its enabling should be tweaked? E.g. I don't think there are any
>>> particularly large or deep nodes in ruby.rb's parse tree. It's a
>>> very shallow file.
> 
> Yeah that is a not-very-clever hack. I’ve got an idea: I can add a C
> function that checks the maximum depth of a parse tree and the maximum
> node span, and turn on the fast-mode if the depth is too large or a node
> is too wide. And we do that check once before doing any fontification.
> 
> I’ll report back once I add it.

Thanks!

And if the check can be fast enough, we could probably do it in the 
beginning of fontifying every chunk.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Eli Zaretskii]

> Cc: 60691@debbugs.gnu.org, juri@linkov.net
> Date: Fri, 13 Jan 2023 01:40:56 +0200
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> Managed to reproduce this after running the test in a couple of 
> different files.
> 
> But 'M-x memory-usage' says no such command, and 'M-x memory-report' 
> ends up with this error:
> 
> Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
>    memory-report--gc-elem(nil strings)
>    memory-report--garbage-collect()
>    memory-report()

This means GC is disabled in this session at the time you invoke
memory-report.  Which shouldn't happen, of course.  It sounds like
your pure Lisp storage overflowed, and that disabled GC.

And I think I see the problem: we use build_pure_c_string in treesit.c
in places that we shouldn't.

Yuan, build_pure_c_string should only be used in places such as
syms_of_treesit, which are called just once, during dumping.  Look at
all the other calls to this function in the sources, and you will see
it.  In all other cases, you should do one of the following:

  . for strings whose text is fixed, define a variable, give it the
    value in syms_of_treesit using build_pure_c_string, then use that
    variable elsewhere in the source
  . for strings whose text depends on run-time information, use
    AUTO_STRING or build_string

This is a serious problem, and we should fix it ASAP.

> garbage-collect's docstring says:
> 
>    However, if there was overflow in pure space, and Emacs was dumped
>    using the "unexec" method, ‘garbage-collect’ returns nil, because
>    real GC can’t be done.
> 
> I don't know if my Emacs was dumped using "unexec", though. ./configure 
> says I'm using pdumper.

The above text doesn't account for bugs ;-)  Functions that produce
objects in pure space are supposed to be called only during the build,
a.k.a. "when dumping", and for that the text is correct.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Yuan Fu]

> On Jan 12, 2023, at 11:57 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Cc: 60691@debbugs.gnu.org, juri@linkov.net
>> Date: Fri, 13 Jan 2023 01:40:56 +0200
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> 
>> Managed to reproduce this after running the test in a couple of 
>> different files.
>> 
>> But 'M-x memory-usage' says no such command, and 'M-x memory-report' 
>> ends up with this error:
>> 
>> Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
>>   memory-report--gc-elem(nil strings)
>>   memory-report--garbage-collect()
>>   memory-report()
> 
> This means GC is disabled in this session at the time you invoke
> memory-report.  Which shouldn't happen, of course.  It sounds like
> your pure Lisp storage overflowed, and that disabled GC.
> 
> And I think I see the problem: we use build_pure_c_string in treesit.c
> in places that we shouldn't.
> 
> Yuan, build_pure_c_string should only be used in places such as
> syms_of_treesit, which are called just once, during dumping.  Look at
> all the other calls to this function in the sources, and you will see
> it.  In all other cases, you should do one of the following:
> 
>  . for strings whose text is fixed, define a variable, give it the
>    value in syms_of_treesit using build_pure_c_string, then use that
>    variable elsewhere in the source

Can I define a bunch of static C variables and initialize them in syms_of_treesit, or they have to be all Lisp variables? Eg,

static Lisp_Object TREESIT_STAR;

...

void
syms_of_treesit (void)
{
...
TREESIT_STAR = build_pure_c_string ("*");
...
}

Yuan

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Fri, 13 Jan 2023 01:15:09 -0800
> Cc: Dmitry Gutov <dgutov@yandex.ru>,
>  60691@debbugs.gnu.org,
>  Juri Linkov <juri@linkov.net>,
>  Stefan Monnier <monnier@iro.umontreal.ca>
> 
> > On Jan 12, 2023, at 11:57 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> > 
> >> Cc: 60691@debbugs.gnu.org, juri@linkov.net
> >> Date: Fri, 13 Jan 2023 01:40:56 +0200
> >> From: Dmitry Gutov <dgutov@yandex.ru>
> >> 
> >> Managed to reproduce this after running the test in a couple of 
> >> different files.
> >> 
> >> But 'M-x memory-usage' says no such command, and 'M-x memory-report' 
> >> ends up with this error:
> >> 
> >> Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
> >>   memory-report--gc-elem(nil strings)
> >>   memory-report--garbage-collect()
> >>   memory-report()
> > 
> > This means GC is disabled in this session at the time you invoke
> > memory-report.  Which shouldn't happen, of course.  It sounds like
> > your pure Lisp storage overflowed, and that disabled GC.
> > 
> > And I think I see the problem: we use build_pure_c_string in treesit.c
> > in places that we shouldn't.
> > 
> > Yuan, build_pure_c_string should only be used in places such as
> > syms_of_treesit, which are called just once, during dumping.  Look at
> > all the other calls to this function in the sources, and you will see
> > it.  In all other cases, you should do one of the following:
> > 
> >  . for strings whose text is fixed, define a variable, give it the
> >    value in syms_of_treesit using build_pure_c_string, then use that
> >    variable elsewhere in the source
> 
> Can I define a bunch of static C variables and initialize them in syms_of_treesit, or they have to be all Lisp variables? Eg,
> 
> static Lisp_Object TREESIT_STAR;
> 
> ...
> 
> void
> syms_of_treesit (void)
> {
> ...
> TREESIT_STAR = build_pure_c_string ("*");
> ...
> }

Yes, of course.  Look, for example, how coding.c does that:

  /* A string that serves as name of the reusable work buffer, and as base
     name of temporary work buffers used for code-conversion operations.  */
  static Lisp_Object Vcode_conversion_workbuf_name;
  [...]
  void
  syms_of_coding (void)
  {
  [...]
    staticpro (&Vcode_conversion_workbuf_name);
    Vcode_conversion_workbuf_name = build_pure_c_string (" *code-conversion-work*");

But please keep the convention of naming such variables Vsome_thing,
both regarding the "V" and the fact that the name is otherwise
lower-case.

Thanks.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Yuan Fu]

> On Jan 13, 2023, at 3:51 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Fri, 13 Jan 2023 01:15:09 -0800
>> Cc: Dmitry Gutov <dgutov@yandex.ru>,
>> 60691@debbugs.gnu.org,
>> Juri Linkov <juri@linkov.net>,
>> Stefan Monnier <monnier@iro.umontreal.ca>
>> 
>>> On Jan 12, 2023, at 11:57 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> 
>>>> Cc: 60691@debbugs.gnu.org, juri@linkov.net
>>>> Date: Fri, 13 Jan 2023 01:40:56 +0200
>>>> From: Dmitry Gutov <dgutov@yandex.ru>
>>>> 
>>>> Managed to reproduce this after running the test in a couple of 
>>>> different files.
>>>> 
>>>> But 'M-x memory-usage' says no such command, and 'M-x memory-report' 
>>>> ends up with this error:
>>>> 
>>>> Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
>>>>  memory-report--gc-elem(nil strings)
>>>>  memory-report--garbage-collect()
>>>>  memory-report()
>>> 
>>> This means GC is disabled in this session at the time you invoke
>>> memory-report.  Which shouldn't happen, of course.  It sounds like
>>> your pure Lisp storage overflowed, and that disabled GC.
>>> 
>>> And I think I see the problem: we use build_pure_c_string in treesit.c
>>> in places that we shouldn't.
>>> 
>>> Yuan, build_pure_c_string should only be used in places such as
>>> syms_of_treesit, which are called just once, during dumping.  Look at
>>> all the other calls to this function in the sources, and you will see
>>> it.  In all other cases, you should do one of the following:
>>> 
>>> . for strings whose text is fixed, define a variable, give it the
>>>   value in syms_of_treesit using build_pure_c_string, then use that
>>>   variable elsewhere in the source
>> 
>> Can I define a bunch of static C variables and initialize them in syms_of_treesit, or they have to be all Lisp variables? Eg,
>> 
>> static Lisp_Object TREESIT_STAR;
>> 
>> ...
>> 
>> void
>> syms_of_treesit (void)
>> {
>> ...
>> TREESIT_STAR = build_pure_c_string ("*");
>> ...
>> }
> 
> Yes, of course.  Look, for example, how coding.c does that:
> 
>  /* A string that serves as name of the reusable work buffer, and as base
>     name of temporary work buffers used for code-conversion operations.  */
>  static Lisp_Object Vcode_conversion_workbuf_name;
>  [...]
>  void
>  syms_of_coding (void)
>  {
>  [...]
>    staticpro (&Vcode_conversion_workbuf_name);
>    Vcode_conversion_workbuf_name = build_pure_c_string (" *code-conversion-work*");
> 
> But please keep the convention of naming such variables Vsome_thing,
> both regarding the "V" and the fact that the name is otherwise
> lower-case.

Thanks, I pushed a fix for it. I also used intern_c_string in some places like these:

intern_c_string (":?”)
intern_c_string (":*")

I want to change them to use DEFSYM, but what should be the c name for them?

Yuan

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Fri, 13 Jan 2023 19:48:40 -0800
> Cc: dgutov@yandex.ru,
>  60691@debbugs.gnu.org,
>  juri@linkov.net,
>  monnier@iro.umontreal.ca
> 
> Thanks, I pushed a fix for it. I also used intern_c_string in some places like these:
> 
> intern_c_string (":?”)
> intern_c_string (":*")
> 
> I want to change them to use DEFSYM, but what should be the c name for them?

Yes, DEFSYM is better in such cases.  The C name can be QCquestion and
QCasterix, for example.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Yuan Fu]

> On Jan 13, 2023, at 11:29 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Fri, 13 Jan 2023 19:48:40 -0800
>> Cc: dgutov@yandex.ru,
>> 60691@debbugs.gnu.org,
>> juri@linkov.net,
>> monnier@iro.umontreal.ca
>> 
>> Thanks, I pushed a fix for it. I also used intern_c_string in some places like these:
>> 
>> intern_c_string (":?”)
>> intern_c_string (":*")
>> 
>> I want to change them to use DEFSYM, but what should be the c name for them?
> 
> Yes, DEFSYM is better in such cases.  The C name can be QCquestion and
> QCasterix, for example.

My worry is that they will conflict with, eg, symbol `question’ and `asterix’, if someone ever defines them in the C codebase. Is that not possible?

Yuan

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Fri, 13 Jan 2023 23:51:05 -0800
> Cc: dgutov@yandex.ru,
>  60691@debbugs.gnu.org,
>  juri@linkov.net,
>  monnier@iro.umontreal.ca
> 
> > Yes, DEFSYM is better in such cases.  The C name can be QCquestion and
> > QCasterix, for example.
> 
> My worry is that they will conflict with, eg, symbol `question’ and `asterix’, if someone ever defines them in the C codebase. Is that not possible?

It's possible, but how can a symbol conflict in that case? it will
just be reused.

But if you want to have treesit-specific symbols, you can use names
like QCasterix_treesit.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Andreas Schwab]

On Jan 13 2023, Yuan Fu wrote:

>> On Jan 13, 2023, at 11:29 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>> From: Yuan Fu <casouri@gmail.com>
>>> Date: Fri, 13 Jan 2023 19:48:40 -0800
>>> Cc: dgutov@yandex.ru,
>>> 60691@debbugs.gnu.org,
>>> juri@linkov.net,
>>> monnier@iro.umontreal.ca
>>> 
>>> Thanks, I pushed a fix for it. I also used intern_c_string in some places like these:
>>> 
>>> intern_c_string (":?”)
>>> intern_c_string (":*")
>>> 
>>> I want to change them to use DEFSYM, but what should be the c name for them?
>> 
>> Yes, DEFSYM is better in such cases.  The C name can be QCquestion and
>> QCasterix, for example.
>
> My worry is that they will conflict with, eg, symbol `question’ and `asterix’, if someone ever defines them in the C codebase. Is that not possible?

The C name of the symbol `question' would be Qquestion, without the C
(which stands for the `:' prefix).

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Yuan Fu]

> On Jan 14, 2023, at 12:46 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> 
> On Jan 13 2023, Yuan Fu wrote:
> 
>>> On Jan 13, 2023, at 11:29 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> 
>>>> From: Yuan Fu <casouri@gmail.com>
>>>> Date: Fri, 13 Jan 2023 19:48:40 -0800
>>>> Cc: dgutov@yandex.ru,
>>>> 60691@debbugs.gnu.org,
>>>> juri@linkov.net,
>>>> monnier@iro.umontreal.ca
>>>> 
>>>> Thanks, I pushed a fix for it. I also used intern_c_string in some places like these:
>>>> 
>>>> intern_c_string (":?”)
>>>> intern_c_string (":*")
>>>> 
>>>> I want to change them to use DEFSYM, but what should be the c name for them?
>>> 
>>> Yes, DEFSYM is better in such cases.  The C name can be QCquestion and
>>> QCasterix, for example.
>> 
>> My worry is that they will conflict with, eg, symbol `question’ and `asterix’, if someone ever defines them in the C codebase. Is that not possible?
> 
> The C name of the symbol `question' would be Qquestion, without the C
> (which stands for the `:' prefix).

Sorry, I meant `:question’, and `:asterix’. 

Yuan

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Yuan Fu]

Yuan Fu <casouri@gmail.com> writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> Yuan? Just making sure you got this message.
>
> Sorry for the delay :-)
>
>> On 10/01/2023 16:10, Dmitry Gutov wrote:
>>> Perhaps Yuan has some further ideas. There are some strong oddities here:
>>> - Some time into debugging and repeating the benchmark again and
>>> again, I get the "Pure Lisp storage overflowed" message. Just once
>>> per Emacs session. It doesn't seem to change much, so it might be
>>> unimportant.
>
> That sounds like 60653. The next time you encounter it, could you record
> the output of M-x memory-usage and M-x memory-report? 
>
>>> - The profiler output looks like this:
>>>    18050  75%                    -
>>> font-lock-fontify-syntactically-region
>>>    15686  65%                     - treesit-font-lock-fontify-region
>>>     3738  15% treesit--children-covering-range-recurse
>>>      188   0%                        treesit-fontify-with-override
>>> - When running the benchmark for the first time in a buffer (such as
>>> ruby.rb), the variable treesit--font-lock-fast-mode is usually
>>> changed to t. In one Emacs session, after I changed it to nil and
>>> re-ran the benchmark, the variable stayed nil, and the benchmark ran
>>> much faster (like 10s vs 36s).
>>> In the next session, after I restarted Emacs, that didn't happen: it
>>> always stayed at t, even if I reset it to nil between runs. But if I
>>> comment out the block in treesit-font-lock-fontify-region that uses
>>> it
>>>      ;; (when treesit--font-lock-fast-mode
>>>      ;;   (setq nodes (treesit--children-covering-range-recurse
>>>      ;;                (car nodes) start end (* 4 jit-lock-chunk-size))))
>>> and evaluate the defun, the benchmark runs much faster again: 11s.
>>> (But then I brought it all back, and re-ran the tests, and the
>>> variable stayed nil that time around; to sum up: the way it's turned
>>> on is unstable.)
>>> Should treesit--font-lock-fast-mode be locally bound inside that
>>> function, so that it's reset between chunks? Or maybe the condition
>>> for its enabling should be tweaked? E.g. I don't think there are any
>>> particularly large or deep nodes in ruby.rb's parse tree. It's a
>>> very shallow file.
>
> Yeah that is a not-very-clever hack. I’ve got an idea: I can add a C
> function that checks the maximum depth of a parse tree and the maximum
> node span, and turn on the fast-mode if the depth is too large or a node
> is too wide. And we do that check once before doing any fontification.
>
> I’ll report back once I add it.

I wrote that function. But I didn’t end up using it. Instead I added a
"grace count", so that the query time has to be longer than the
threshold 5 times before we switch on the fast mode instead of 1.

My main worry is that simply looking at the parse tree would not catch
all the case where there will be expensive queries.

Could you try the latest commit and see if the fast mode still switches
on when it shouldn’t?

Yuan

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Dmitry Gutov]

Hi Yuan,

On 18/01/2023 08:50, Yuan Fu wrote:
>>>> Should treesit--font-lock-fast-mode be locally bound inside that
>>>> function, so that it's reset between chunks? Or maybe the condition
>>>> for its enabling should be tweaked? E.g. I don't think there are any
>>>> particularly large or deep nodes in ruby.rb's parse tree. It's a
>>>> very shallow file.
>>
>> Yeah that is a not-very-clever hack. I’ve got an idea: I can add a C
>> function that checks the maximum depth of a parse tree and the maximum
>> node span, and turn on the fast-mode if the depth is too large or a node
>> is too wide. And we do that check once before doing any fontification.
>>
>> I’ll report back once I add it.
> 
> I wrote that function. But I didn’t end up using it. Instead I added a
> "grace count", so that the query time has to be longer than the
> threshold 5 times before we switch on the fast mode instead of 1.
> 
> My main worry is that simply looking at the parse tree would not catch
> all the case where there will be expensive queries.

That might be true, but a criterion that doesn't specify conditions 
exactly can give no guarantee against false positives.

> Could you try the latest commit and see if the fast mode still switches
> on when it shouldn’t?

At first it seemed to help, but then I switched the major mode a couple 
more times, and ran the benchmark twice more, and the "fast mode" 
switched on again.

Which seems to make sense: there is no resetting the counter, right?

So if previously it happened once somehow during a certain scenario, now 
I have to repeat the same scenario 4 times, and the condition is met.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Yuan Fu]

> On Jan 19, 2023, at 10:28 AM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> 
> Hi Yuan,
> 
> On 18/01/2023 08:50, Yuan Fu wrote:
>>>>> Should treesit--font-lock-fast-mode be locally bound inside that
>>>>> function, so that it's reset between chunks? Or maybe the condition
>>>>> for its enabling should be tweaked? E.g. I don't think there are any
>>>>> particularly large or deep nodes in ruby.rb's parse tree. It's a
>>>>> very shallow file.
>>> 
>>> Yeah that is a not-very-clever hack. I’ve got an idea: I can add a C
>>> function that checks the maximum depth of a parse tree and the maximum
>>> node span, and turn on the fast-mode if the depth is too large or a node
>>> is too wide. And we do that check once before doing any fontification.
>>> 
>>> I’ll report back once I add it.
>> I wrote that function. But I didn’t end up using it. Instead I added a
>> "grace count", so that the query time has to be longer than the
>> threshold 5 times before we switch on the fast mode instead of 1.
>> My main worry is that simply looking at the parse tree would not catch
>> all the case where there will be expensive queries.
> 
> That might be true, but a criterion that doesn't specify conditions exactly can give no guarantee against false positives.

The condition is “query is (consistently) slow”, that’s why I thought measuring the time is the most direct way.

> 
>> Could you try the latest commit and see if the fast mode still switches
>> on when it shouldn’t?
> 
> At first it seemed to help, but then I switched the major mode a couple more times, and ran the benchmark twice more, and the "fast mode" switched on again.
> 
> Which seems to make sense: there is no resetting the counter, right?
> 
> So if previously it happened once somehow during a certain scenario, now I have to repeat the same scenario 4 times, and the condition is met.

I was hoping that the scenario only happen once, oh well :-) I’ll change the decision based on analyzing the tree’s dimension: too deep or too wide activates the fast mode. Let’s see how it works.

Yuan

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Dmitry Gutov]

On 21/01/2023 00:24, Yuan Fu wrote:
> 
> 
>> On Jan 19, 2023, at 10:28 AM, Dmitry Gutov <dgutov@yandex.ru> wrote:
>>
>> Hi Yuan,
>>
>> On 18/01/2023 08:50, Yuan Fu wrote:
>>>>>> Should treesit--font-lock-fast-mode be locally bound inside that
>>>>>> function, so that it's reset between chunks? Or maybe the condition
>>>>>> for its enabling should be tweaked? E.g. I don't think there are any
>>>>>> particularly large or deep nodes in ruby.rb's parse tree. It's a
>>>>>> very shallow file.
>>>>
>>>> Yeah that is a not-very-clever hack. I’ve got an idea: I can add a C
>>>> function that checks the maximum depth of a parse tree and the maximum
>>>> node span, and turn on the fast-mode if the depth is too large or a node
>>>> is too wide. And we do that check once before doing any fontification.
>>>>
>>>> I’ll report back once I add it.
>>> I wrote that function. But I didn’t end up using it. Instead I added a
>>> "grace count", so that the query time has to be longer than the
>>> threshold 5 times before we switch on the fast mode instead of 1.
>>> My main worry is that simply looking at the parse tree would not catch
>>> all the case where there will be expensive queries.
>>
>> That might be true, but a criterion that doesn't specify conditions exactly can give no guarantee against false positives.
> 
> The condition is “query is (consistently) slow”, that’s why I thought measuring the time is the most direct way.

The benchmark itself might be artificial, in that it's measuring the 
font-lock of a specific buffer, in whole, for 1000 iterations. But Juri 
must have come up with the original report based on real usage scenario.

OTOH, the scenario which it might correspond to, is used typing in the 
same buffer for a long time (triggering thousands of refontifications, 
possibly partial ones). I don't know if it's feasible to try to 
reproduce it specifically. But, again, anything that can happen once can 
happen 4 more times.

>>> Could you try the latest commit and see if the fast mode still switches
>>> on when it shouldn’t?
>>
>> At first it seemed to help, but then I switched the major mode a couple more times, and ran the benchmark twice more, and the "fast mode" switched on again.
>>
>> Which seems to make sense: there is no resetting the counter, right?
>>
>> So if previously it happened once somehow during a certain scenario, now I have to repeat the same scenario 4 times, and the condition is met.
> 
> I was hoping that the scenario only happen once, oh well :-) I’ll change the decision based on analyzing the tree’s dimension: too deep or too wide activates the fast mode. Let’s see how it works.

Thank you, let me know when it's time to test again.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Yuan Fu]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 21/01/2023 00:24, Yuan Fu wrote:
>> 
>>> On Jan 19, 2023, at 10:28 AM, Dmitry Gutov <dgutov@yandex.ru> wrote:
>>>
>>> Hi Yuan,
>>>
>>> On 18/01/2023 08:50, Yuan Fu wrote:
>>>>>>> Should treesit--font-lock-fast-mode be locally bound inside that
>>>>>>> function, so that it's reset between chunks? Or maybe the condition
>>>>>>> for its enabling should be tweaked? E.g. I don't think there are any
>>>>>>> particularly large or deep nodes in ruby.rb's parse tree. It's a
>>>>>>> very shallow file.
>>>>>
>>>>> Yeah that is a not-very-clever hack. I’ve got an idea: I can add a C
>>>>> function that checks the maximum depth of a parse tree and the maximum
>>>>> node span, and turn on the fast-mode if the depth is too large or a node
>>>>> is too wide. And we do that check once before doing any fontification.
>>>>>
>>>>> I’ll report back once I add it.
>>>> I wrote that function. But I didn’t end up using it. Instead I added a
>>>> "grace count", so that the query time has to be longer than the
>>>> threshold 5 times before we switch on the fast mode instead of 1.
>>>> My main worry is that simply looking at the parse tree would not catch
>>>> all the case where there will be expensive queries.
>>>
>>> That might be true, but a criterion that doesn't specify conditions exactly can give no guarantee against false positives.
>> The condition is “query is (consistently) slow”, that’s why I
>> thought measuring the time is the most direct way.
>
> The benchmark itself might be artificial, in that it's measuring the
> font-lock of a specific buffer, in whole, for 1000 iterations. But
> Juri must have come up with the original report based on real usage
> scenario.
>
> OTOH, the scenario which it might correspond to, is used typing in the
> same buffer for a long time (triggering thousands of refontifications,
> possibly partial ones). I don't know if it's feasible to try to
> reproduce it specifically. But, again, anything that can happen once
> can happen 4 more times.
>
>>>> Could you try the latest commit and see if the fast mode still switches
>>>> on when it shouldn’t?
>>>
>>> At first it seemed to help, but then I switched the major mode a
>>> couple more times, and ran the benchmark twice more, and the "fast
>>> mode" switched on again.
>>>
>>> Which seems to make sense: there is no resetting the counter, right?
>>>
>>> So if previously it happened once somehow during a certain scenario, now I have to repeat the same scenario 4 times, and the condition is met.
>> I was hoping that the scenario only happen once, oh well :-) I’ll
>> change the decision based on analyzing the tree’s dimension: too
>> deep or too wide activates the fast mode. Let’s see how it works.
>
> Thank you, let me know when it's time to test again.

Sorry for the delay. Now treesit-font-lock-fontify-region uses
treesit-subtree-stat to determine whether to enable the "fast mode". Now
it should be impossible to activate the fast mode on moderately sized
buffers.

Yuan

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Dmitry Gutov]

Hi Yuan,

On 29/01/2023 10:25, Yuan Fu wrote:

>>>> So if previously it happened once somehow during a certain scenario, now I have to repeat the same scenario 4 times, and the condition is met.
>>> I was hoping that the scenario only happen once, oh well :-) I’ll
>>> change the decision based on analyzing the tree’s dimension: too
>>> deep or too wide activates the fast mode. Let’s see how it works.
>>
>> Thank you, let me know when it's time to test again.
> 
> Sorry for the delay. Now treesit-font-lock-fontify-region uses
> treesit-subtree-stat to determine whether to enable the "fast mode". Now
> it should be impossible to activate the fast mode on moderately sized
> buffers.

Thank you, it seems to work just fine in my scenario. And 
treesit-subtree-stat makes sense.

I have a few more questions about the current strategy, though.

IIUC, we only do the treesit--font-lock-fast-mode test once in 
treesit-font-lock-fontify-region, and then use the detected value for 
the whole later life of the buffer. Is that right?

What if the buffer didn't originally have the problematic error nodes we 
are guarding from, and then later the user wrote enough code to have at 
least one of them? If they didn't close Emacs, or revert the buffer, our 
logic still wouldn't use the "fast node", would it?

Or vice versa: if the buffer started out with error nodes, and 
consequently, "fast mode", but then the user has edited it so that those 
error nodes disappeared, shouldn't the buffer stop using the "fast mode"?

 From my measurements, in ruby-mode, at least treesit-subtree-stat is 
20-40x faster than refontifying the whole buffer. So one possible 
strategy would be to repeat the test every time. I'm not sure it's fast 
enough in the "problem" buffers, though, and I don't have any to test.

In those I did test, though, it takes ~1 ms.

But we could repeat the test only once every couple of seconds and/or 
after the buffer has changed again. That would hopefully make it a 
non-bottleneck in all cases.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Yuan Fu]

> On Jan 29, 2023, at 3:07 PM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> 
> Hi Yuan,
> 
> On 29/01/2023 10:25, Yuan Fu wrote:
> 
>>>>> So if previously it happened once somehow during a certain scenario, now I have to repeat the same scenario 4 times, and the condition is met.
>>>> I was hoping that the scenario only happen once, oh well :-) I’ll
>>>> change the decision based on analyzing the tree’s dimension: too
>>>> deep or too wide activates the fast mode. Let’s see how it works.
>>> 
>>> Thank you, let me know when it's time to test again.
>> Sorry for the delay. Now treesit-font-lock-fontify-region uses
>> treesit-subtree-stat to determine whether to enable the "fast mode". Now
>> it should be impossible to activate the fast mode on moderately sized
>> buffers.
> 
> Thank you, it seems to work just fine in my scenario. And treesit-subtree-stat makes sense.
> 
> I have a few more questions about the current strategy, though.
> 
> IIUC, we only do the treesit--font-lock-fast-mode test once in treesit-font-lock-fontify-region, and then use the detected value for the whole later life of the buffer. Is that right?
> 
> What if the buffer didn't originally have the problematic error nodes we are guarding from, and then later the user wrote enough code to have at least one of them? If they didn't close Emacs, or revert the buffer, our logic still wouldn't use the "fast node", would it?
> 
> Or vice versa: if the buffer started out with error nodes, and consequently, "fast mode", but then the user has edited it so that those error nodes disappeared, shouldn't the buffer stop using the "fast mode"?
> 
> From my measurements, in ruby-mode, at least treesit-subtree-stat is 20-40x faster than refontifying the whole buffer. So one possible strategy would be to repeat the test every time. I'm not sure it's fast enough in the "problem" buffers, though, and I don't have any to test.
> 
> In those I did test, though, it takes ~1 ms.
> 
> But we could repeat the test only once every couple of seconds and/or after the buffer has changed again. That would hopefully make it a non-bottleneck in all cases.

I should mention this in the comments, but the fast mode is only for very rare cases, where the file is mechanically generated and has some peculiarities that causes tree-sitter to work poorly. If the file is hand-written and “normal”, even huge files like xdisp.c is well below the bar. Therefore I don’t think “crossing the line” will realistically happen when editing source files.

Here is the stats of two “problematic files”, named packet and dec_mask, comparing to xdisp.c:

;;           max-depth max-width count
;; cut-off   100       4000
;; packet   (98159     46581 1895137)
;; dec mask (3         64301 283995)
;; xdisp.c  (29        985   218971)

I’d say that any regular source file, even mechanically generated, wouldn’t go beyond ~50 levels in depth, and hand-written files should never has a node that has 4000+ direct children in the parse tree.

Yuan

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Dmitry Gutov]

On 30/01/2023 01:23, Yuan Fu wrote:
> 
>> On Jan 29, 2023, at 3:07 PM, Dmitry Gutov<dgutov@yandex.ru>  wrote:
>>
>> Hi Yuan,
>>
>> On 29/01/2023 10:25, Yuan Fu wrote:
>>
>>>>>> So if previously it happened once somehow during a certain scenario, now I have to repeat the same scenario 4 times, and the condition is met.
>>>>> I was hoping that the scenario only happen once, oh well 😄 I’ll
>>>>> change the decision based on analyzing the tree’s dimension: too
>>>>> deep or too wide activates the fast mode. Let’s see how it works.
>>>> Thank you, let me know when it's time to test again.
>>> Sorry for the delay. Now treesit-font-lock-fontify-region uses
>>> treesit-subtree-stat to determine whether to enable the "fast mode". Now
>>> it should be impossible to activate the fast mode on moderately sized
>>> buffers.
>> Thank you, it seems to work just fine in my scenario. And treesit-subtree-stat makes sense.
>>
>> I have a few more questions about the current strategy, though.
>>
>> IIUC, we only do the treesit--font-lock-fast-mode test once in treesit-font-lock-fontify-region, and then use the detected value for the whole later life of the buffer. Is that right?
>>
>> What if the buffer didn't originally have the problematic error nodes we are guarding from, and then later the user wrote enough code to have at least one of them? If they didn't close Emacs, or revert the buffer, our logic still wouldn't use the "fast node", would it?
>>
>> Or vice versa: if the buffer started out with error nodes, and consequently, "fast mode", but then the user has edited it so that those error nodes disappeared, shouldn't the buffer stop using the "fast mode"?
>>
>>  From my measurements, in ruby-mode, at least treesit-subtree-stat is 20-40x faster than refontifying the whole buffer. So one possible strategy would be to repeat the test every time. I'm not sure it's fast enough in the "problem" buffers, though, and I don't have any to test.
>>
>> In those I did test, though, it takes ~1 ms.
>>
>> But we could repeat the test only once every couple of seconds and/or after the buffer has changed again. That would hopefully make it a non-bottleneck in all cases.
> I should mention this in the comments, but the fast mode is only for very rare cases, where the file is mechanically generated and has some peculiarities that causes tree-sitter to work poorly. If the file is hand-written and “normal”, even huge files like xdisp.c is well below the bar. Therefore I don’t think “crossing the line” will realistically happen when editing source files.
> 
> Here is the stats of two “problematic files”, named packet and dec_mask, comparing to xdisp.c:
> 
> ;;           max-depth max-width count
> ;; cut-off   100       4000
> ;; packet   (98159     46581 1895137)
> ;; dec mask (3         64301 283995)
> ;; xdisp.c  (29        985   218971)
> 
> I’d say that any regular source file, even mechanically generated, wouldn’t go beyond ~50 levels in depth, and hand-written files should never has a node that has 4000+ direct children in the parse tree.

Oh, thanks for the explanation. Then the current strategy makes sense.

Is xdisp.c absolutely the largest C file in your experience?

According to the above numbers, a file that's only 4x as large could hit 
our current cutoff.

Though, TBH, maybe some extreme files do, and they have font-lock 
performance reduced somewhat. That's not the end of the world, and it 
shouldn't make a difference for the original scenario (diff-syntax 
fontification).

Either way, I'm closing this report. Thank you for your help.

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Yuan Fu]

>> I should mention this in the comments, but the fast mode is only for very rare cases, where the file is mechanically generated and has some peculiarities that causes tree-sitter to work poorly. If the file is hand-written and “normal”, even huge files like xdisp.c is well below the bar. Therefore I don’t think “crossing the line” will realistically happen when editing source files.
>> Here is the stats of two “problematic files”, named packet and dec_mask, comparing to xdisp.c:
>> ;;           max-depth max-width count
>> ;; cut-off   100       4000
>> ;; packet   (98159     46581 1895137)
>> ;; dec mask (3         64301 283995)
>> ;; xdisp.c  (29        985   218971)
>> I’d say that any regular source file, even mechanically generated, wouldn’t go beyond ~50 levels in depth, and hand-written files should never has a node that has 4000+ direct children in the parse tree.
> 
> Oh, thanks for the explanation. Then the current strategy makes sense.
> 
> Is xdisp.c absolutely the largest C file in your experience?
> 
> According to the above numbers, a file that's only 4x as large could hit our current cutoff.

I don’t think these stats increase linearly as the file size increases. Even if there is a file that has a node with 3999 direct children, and the developer adds another one, I’d say it’s better not to turn on “fast mode” immediately.

Yuan

bug#60691: 29.0.60; Slow tree-sitter font-lock in ruby-ts-mode

[Dmitry Gutov]

On 01/02/2023 07:26, Yuan Fu wrote:
>>> I should mention this in the comments, but the fast mode is only for very rare cases, where the file is mechanically generated and has some peculiarities that causes tree-sitter to work poorly. If the file is hand-written and “normal”, even huge files like xdisp.c is well below the bar. Therefore I don’t think “crossing the line” will realistically happen when editing source files.
>>> Here is the stats of two “problematic files”, named packet and dec_mask, comparing to xdisp.c:
>>> ;;           max-depth max-width count
>>> ;; cut-off   100       4000
>>> ;; packet   (98159     46581 1895137)
>>> ;; dec mask (3         64301 283995)
>>> ;; xdisp.c  (29        985   218971)
>>> I’d say that any regular source file, even mechanically generated, wouldn’t go beyond ~50 levels in depth, and hand-written files should never has a node that has 4000+ direct children in the parse tree.
>> Oh, thanks for the explanation. Then the current strategy makes sense.
>>
>> Is xdisp.c absolutely the largest C file in your experience?
>>
>> According to the above numbers, a file that's only 4x as large could hit our current cutoff.
> I don’t think these stats increase linearly as the file size increases. Even if there is a file that has a node with 3999 direct children, and the developer adds another one, I’d say it’s better not to turn on “fast mode” immediately.

I see your point.

In the previous message I was talking about a different scenario: when a 
project has a file 4x the size of xdisp.c, and the user just opens it. I 
suspect it's not great to have "fast mode" enabled in that case? Like, 
false positive.

Anyway, this is a very theoretical concern on my part.