semantic-tokens: cache modifier lookups

[alanz]

Make a buffer-local cache of the mapping from a modifier bitmap to the
set of faces it resolves to.

The assumption is that for a given language the modifier usage will
tend to cluster, so the lookup table will not be large.

[ericdallo]

c/c @sebastiansturm

[sebastiansturm]

Hi @alanz, thanks for your PR!
I tried it out today, but so far haven't seen performance improvements; on the contrary, performance seemed to get worse in the one scenario I tested. To be precise, I cloned the latest rust-analyzer from source and, within crates/hir-def/src/nameres/collector.rs, executed the following function (using a very recent Emacs 29 build, with lsp-mode natively compiled in both cases):

(defun benchmark/fontification (count) ()
       (garbage-collect)
       (benchmark-call (lambda () (lsp-semantic-tokens--fontify #'font-lock-fontify-region (point-min) (point-max))) count))

The results of 5 consecutive evaluations of (benchmark/fontification 50) were as follows:

current lsp-mode master:
(4.812272216 3 0.34256550100000016)
(4.8488806 3 0.3737376499999998)
(4.849987764000001 3 0.3759672280000004)
(4.850168844 3 0.3760423690000003)
(4.843063498999999 3 0.3765382319999997)

PR 3670:
(5.1777232269999995 6 0.536172971)
(5.1809434 6 0.5332634989999998)
(5.182143025 6 0.5344310930000002)
(5.177091014 6 0.5340501979999996)
(5.180054484 6 0.5352050740000003)

One obvious pessimization (that you already remarked on in a comment, but AFAICS didn't address yet) is that empty modifier face sets will fail the unless faces-to-apply check and thus get recomputed every time. Perhaps fixing that will lead to actual performance improvements? Or maybe you've been testing a different use case that already benefits from the PR in its current form; if that is the case, what benchmarks should I perform to reproduce your results?

[alanz]

Thanks for taking a look @sebastiansturm .

I am actually using it from my https://github.com/alanz/lsp-mode/tree/semantic-modifiers-optimise branch, which is basically this PR on top of #3668, which fully populates the modifier faces for rust-analyzer.

And I put the optimisation in pace because otherwise navigating in rust files such as rust-analyzer main_loop.rs was terribly slow. I did not benchmark it, but found a noticeable sluggishness doing something simple like going to the top of that file (once the server was fully started up), and just moving down line by line with my cursor. It is not nearly so sluggish using this change.

[alanz]

I just did those benchmarks on my own machine with emacs 28.1, jit enabled, without precompiling lsp-rust.el or lsp-semantic-tokens.el.

I get

On my semantic-modifiers-optimise branch
With bad font specifications
(0.003357608 0 0.0)
(0.003357608 0 0.0)
(0.003357608 0 0.0)

Without the semantic token optimisation
With bad font specifications
(18.839329314 32 1.6363554149999997)
(18.839329314 32 1.6363554149999997)

Same, but with good font specifications
(18.737020591 32 1.626868912)
(18.620734945 32 1.61827597)
(18.620734945 32 1.61827597)

My Messages was littered with missing font declarations, so I changed lsp-rust.el to have

(defun lsp-rust-analyzer--set-tokens ()
  "Set the mapping between rust-analyzer keywords and fonts to apply.
The keywords are sent in the initialize response, in the semantic
tokens legend."
  (setq lsp-semantic-token-modifier-faces
        '(
          ("documentation" . font-lock-nop-face)
          ("declaration" . font-lock-nop-face)
          ("definition" . font-lock-nop-face)
          ("static" . font-lock-nop-face)
          ("abstract" . font-lock-nop-face)
          ("deprecated" . font-lock-nop-face)
          ("readonly" . font-lock-nop-face)
          ("default_library" . font-lock-nop-face)
          ("async" . font-lock-nop-face)
          ("attribute" . font-lock-nop-face)
          ("callable" . font-lock-nop-face)
          ("constant" . font-lock-nop-face)
          ("consuming" . font-lock-nop-face)
          ("control_flow" . font-lock-nop-face)
          ("crate_root" . font-lock-nop-face)
          ("injected" . font-lock-nop-face)
          ("intra_doc_link" . font-lock-nop-face)
          ("library" . font-lock-nop-face)
          ("mutable" . font-lock-mutable-modifier-face)
          ("public" . font-lock-nop-face)
          ("reference" . font-lock-reference-modifier-face)
          ("trait" . font-lock-nop-face)
          ("unsafe" . font-lock-nop-face)
          )))

Which shut them up, but made no difference to the performance.

So for me it makes a big difference to use this PR

[alanz]

I revisited this tonight, and redid my benchmarks. And realised my first result in #3670 (comment) is bogus.

Herewith revised benchmark results

Benchmark, on collector.rs
(benchmark/fontification 50)

My updated PR 3668 (lsp-rust semantic modifiers, this PR not applied)
(18.640859705 28 1.5243770410000002)
(18.620477035 28 1.5261523759999998)
(18.438429331 28 1.5186607939999996)

My caching, on top of PR 3668. State of this PR as of yesterday
(14.82362226 54 2.765928829)
(14.771710461000001 53 2.7048730019999994)
(14.812715127 53 2.7507432979999997)

My improved caching, on top of PR 3668, state as of now
(8.524490870000001 43 2.191504358)
(8.454953522 42 2.112336408)
(8.417772501 42 2.1134970299999996)

The change I made was to supply a symbol for the not-found state in the hashmap lookup to distinguish between not cached and no fonts applicable.

And with that it goes roughly twice as fast as without it.

[sebastiansturm]

thanks! Makes sense that caching would help with that long list of modifiers, and might be a good idea in general, now that the no-modifiers case is no longer being pessimized. Adding a few inline comments now...

[sebastiansturm]

yes, I guess it would make sense to store the modifiers cache as part of the workspace, cf. lsp--semantic-tokens-initialize-workspace (which initializes lsp--workspace-semantic-tokens-faces and lsp--workspace-semantic-tokens-modifier-faces depending on token capabilities reported by the server)

[alanz]

I am leaving this as a follow-up, given that the current implementation looks for whichever first random client has a value. Updating it could be tricky:

Line 476

        (modifier-faces
         (when lsp-semantic-tokens-apply-modifiers
           (seq-some #'lsp--workspace-semantic-tokens-modifier-faces lsp--buffer-workspaces)))

[sebastiansturm]

true, at some point I should probably also permit using several semhl-capable lsp servers simultaneously; though I wonder how useful that is in practice (does anyone use several servers at once, with more than one of them providing semantic highlights?)

[sebastiansturm]

I think this is a bit faster than mapc + lambda (could you please benchmark this?):

(dolist (face faces-to-apply) (add-face-text-property text-property-beg text-property-end face))

[alanz]

It makes a miniscule difference

Benchmark, on collector.rs
(benchmark/fontification 50)

My improved caching, on top of PR 3668
(8.577711427999999 43 2.15895247)
(8.576204198000001 43 2.1617159800000003)
(8.585730497000000 43 2.1770649470000007)

With dolist
(8.706144393999999 44 2.205544484)
(8.598737171000000 43 2.1419443979999997)
(8.361974631000000 41 2.0108028530000004)
(8.385988765000000 41 2.001014791000001)

I was wondering if we could do the combining of all those faces before putting them into the cache, so we only need to apply the combination once per token.

[sebastiansturm]

I wondered about that too, but I didn't see anything akin to add-face-text-property that would accept a font list (and setting the face property directly would conflict with other font-lock mechanisms that might be used besides semantic tokens). Maybe such a method does exist, or maybe it might be worthwhile to convert the face list into a face attribute plist which could then be passed to add-face-text-property (though we'd then have to invalidate the cache any time a different theme is selected).
For now, I think the current implementation is fine and it seems to work as intended, so I'll approve this as-is, then we can merge #3680 and your pr #3668 on top of that