Unnecessary refreshing / incorrect application of dynamic font-lock

[yuhan0]

Expected behavior

Cider precompiles font lock keywords per buffer for dynamic highlighting of the macros / deprecated / instrumented vars etc. in scope.

This can be an expensive operation, and should be recomputed at minimum when the buffer's namespace or its requires have changed, and perhaps even delayed when the buffer is not visible.

Dynamic font lock should not apply to any config files like project.clj, edn files, and various other background temp buffers.

Actual behavior

After certain commands (jack-in, eval, load-file), cider-refresh-dynamic-font-lock gets called indiscrimately on background buffers that do not require refreshing. With large numbers of clojure-mode buffers open in the background, this leads to periods of Emacs freezing up.

The main causes are

• cider-enable-on-existing-clojure-buffers (separate issue created: Deprecate or change default setting for cider-auto-mode #3346)

• cider-repl--state-handler not distinguishing between namespaces in different connections.

Since clojure.core is a universally referred ns, an initial connection will trigger a simultaneous refresh in all background buffers. Subsequent evals may also cause refreshes in buffers in unrelated connections that share dependencies with overlapping ns names. This manifests as regular eval commands periodically freezing up for a few seconds for no apparent reason, and then behaving normally afterwards.

The user ns is especially affected, since it is the default for *cider-repl* connection buffers. The use of (cider-current-ns) also causes config files like deps.edn / project.clj to be reported as user, this causes vars from the user ns to be incorrectly font-locked in these contexts.

Steps to reproduce the problem

;; the default config:
(setq cider-font-lock-dynamically '(macro core deprecated)) 

Open hundreds of .clj files in project A with a connected REPL, then switch to project B and jack into it from a single file buffer.

There should be a noticeable period of the interface freezing up after the new REPL appears and begins its initialization, refreshing the font-lock in all the background project A buffers.

---

In the *cider-repl* buffer, enter

user> (def ^:deprecated oops)
;; => #'user/oops

Navigate to a deps.edn / project.clj in either project A or B, and observe that the symbol oops is incorrectly highlighted with cider-deprecated-face.

Environment & Version information

CIDER version information

;; CIDER 1.7.0 (Côte d'Azur), nREPL 1.0.0
;; Clojure 1.11.1, Java 20

Lein / Clojure CLI version

Clojure CLI version 1.11.1.1273

Emacs version

GNU Emacs 29.0.90

Operating system

macOS 12.5.1

JDK distribution

Temurin jdk-20.0.1+9

[vemv]

I read the issue a few times and I'm not sure it's clear to me. Not that I'm intimately familiar with this code.

Could this be a mixture of "works as designed", "Sesman issues" and "problem that it's only noticeable at edge cases"? (you put as example opening hundreds of files and then switching projects)

I'm not trying do discredit the problem statement - I just seek greater understanding.

Could you give specific examples of the different scenario combinations and the behavior you'd expect for them? If possible, make a table.

(Rough example: if I make code change x, for ns structure y, I <should / should not> see a font locking refresh)

Lastly, is there the possibility that fixing Sesman (with both the short-term bandaid and the longer-term refactoring) would also fix these pains? Sometimes larger-scoped optimizations make other, comparatively smaller problems disappear.

Cheers - V

[yuhan0]

Right, here's a simpler concrete repro. I've edited the following functions to print messages in a *trace* buffer for easier understanding of what's going on: https://gist.github.com/yuhan0/5bfcbc585377eefb7cf09a356b961ffa

Refreshing <buffer> lines indicate that font-lock keywords are being recompiled in that buffer, with a 'header' indicating the cause.

---

Setup two separate projects A and B:

A/
  deps.edn     -> {:paths ["src"] :x oops}
  src/foo.clj  -> (ns foo)
B/
  deps.edn     -> (:paths ["src"] :x oops}
  src/bar.clj  -> (ns bar) 

Open buffers A/foo.clj and B/bar.clj, then jack in from project A

C-x b B/bar.clj
C-x b A/foo.clj
M-x cider-jack-in

Trace:

Setting ns to: user
  Refreshing: *cider-repl scratch/A:localhost:60737(clj)*

Setting ns to: user
  Refreshing: *cider-repl scratch/A:localhost:60737(clj)*

Re-enabling Cider
  Refreshing: foo.clj
  Refreshing: bar.clj

Handling state for: *cider-repl scratch/A:localhost:60737(clj)*
Changed namespaces: (clojure.core user)
  Refreshing: *cider-repl scratch/A:localhost:60737(clj)*
  Refreshing:  *cider-temp clojure-mode*

• Note: cider-set-buffer-ns gets called twice on repl init, but this is not relevant here.
• The repl buffer is refreshed 3 times in the course of initialization, again not a big deal.
• Both foo.clj and bar.clj are unnecessarily refreshed due to cider-auto-mode calling (cider-mode +1) on them. With many more open buffers this adds up to a significant UI freeze. See other issue Deprecate or change default setting for cider-auto-mode #3346.

---

With A in the background, jack into a separate connection from project B:

C-x b bar.clj
M-x cider-jack-in

Trace:

Setting ns to: user
  Refreshing: *cider-repl scratch/B:localhost:60742(clj)*

Setting ns to: user
  Refreshing: *cider-repl scratch/B:localhost:60742(clj)*

Re-enabling Cider
  Refreshing: bar.clj
  Refreshing: foo.clj
  Refreshing:  *cider-temp clojure-mode*

Handling state for: *cider-repl scratch/B:localhost:60742(clj)*
Changed namespaces: (clojure.core user)
  Refreshing:  *cider-temp clojure-mode*
  Refreshing: *cider-repl scratch/A:localhost:60737(clj)*
  Refreshing: *cider-repl scratch/B:localhost:60742(clj)*

• Again cider-auto-mode reinitializes all background buffers, this time including a temp buffer used for fontification.
• Now the state handler is notified of changes to the user ns in project B.
  • Project A's repl should not see a refresh, but the logic does not distinguish overlapping namespaces from different connections.
  • The temp buffer should not see a refresh but it is considered to be in user by (cider-current-ns)

---

Now open both deps.edn files, and create a deprecated var in Project B's user namespace.

C-x C-f A/deps.edn
C-x C-f B/deps.edn
M-x cider-switch-to-repl-buffer

Evaluate the following

user> (def ^:deprecated oops)
;; => #'user/oops

• Navigate to both edn files and observe that oops is highlighted with cider-deprecated-face.
• Navigate to the A repl buffer and type oops in the prompt, observe that it is also highlighted even though the var does not exist in that repl.

Trace:

Setting ns to: user
  Refreshing: *cider-repl scratch/B:localhost:60742(clj)*

Handling state for: *cider-repl scratch/B:localhost:60742(clj)*
Changed namespaces: (clojure.core user)
  Refreshing: *cider-repl scratch/B:localhost:60742(clj)*
  Refreshing: b/deps.edn
  Refreshing: a/deps.edn
  Refreshing:  *cider-temp clojure-mode*
  Refreshing: *cider-repl scratch/A:localhost:60737(clj)*

• Only the B repl buffer should see a refresh.
• The A repl buffer belongs to a different connection and should not see a refresh
• Both deps.edn buffers and the temp buffer are refreshed because they return user as their ns.

[yuhan0]

Lastly, is there the possibility that fixing Sesman (with both the short-term bandaid and the longer-term refactoring) would also fix these pains? Sometimes larger-scoped optimizations make other, comparatively smaller problems disappear.

Yes, I suggested this indirectly in the original issue, that if the Sesman API had a perfomant method of mapping sessions -> buffers, eg. (sesman-get-buffers (cider-current-session)), that would ease implementing the correct logic in the state handler.
This issue could be fixed in the meantime by filtering the buffer list for those returning the same (cider-current-repl) as the REPL calling the state-handler, but that would add more overhead from session inference.

[vemv]

Right, here's a simpler concrete repro.

Thanks much! Repro makes sense to me.

I don't know about the deps.edn <=> user ns problem. It seems very minor to me. It makes sense to consider deps.edn or project.clj belong to user (in the Lein case, project.clj's namespace is literally user).

One wouldn't define deprecated vars in the user namespace anyway. And also, in the grand scheme of things, if we optimize the "99% case" (all buffers but deps.edn get optimal refreshing behavior) we'd be much happy.

Feel free to propose a specific solution to the repro's main problem.

I do plan to work on Sesman very soon. As mentioned we have two foreseen phases:

• short-term bandaids / short-circuiting
  • aka a feature flag
• longer-term Sesman refactoring and maintenance
  • probably we'll end up inlining this lib into this repo

Probably the first phase overlaps (at least partly) with the sort of things you want to accomplish. Would you be up for it?

I can create an issue and @ you.

[yuhan0]

I don't know about the deps.edn <=> user ns problem. It seems very minor to me. It makes sense to consider deps.edn or project.clj belong to user (in the Lein case, project.clj's namespace is literally user).

Yes, it's ultimately a minor nitpicky issue :P I didn't realise that Lein's project.clj has an implicit user ns, but logically EDN files are just data and shouldn't have anything to do with namespaces.
A simple 99% patch to cider-current-ns might be something like (if (string-match "\\.clj" (buffer-file-name)) "user" nil) which rules out any temp buffers and edn files.

(BTW, I used a deprecated var just to demonstrate the issue with a prominent looking face, notice eg. that the symbol map in edn files is highlighted with font-lock-function-name-face after a repl connect)

I'd be happy to contribute to some short term fixes, feel free to tag me in any related discussions 👍

[vemv]

A simple 99% patch to cider-current-ns might be something like (if (string-match "\.clj" (buffer-file-name)) "user" nil) which rules out any temp buffers and edn files.

That might be some unintended effect. After all, even if we're visiting a .edn browser, we might still want to eval things (like throwaway Clojure code) or use interactive functionality (like cider-format-buffer).

What I intend to mean is, a buffer that is running clojure-mode should always map to some ns, not nil. user is a reasonable default.

However, do feel free to come up with any fix such that .edn files don't get dynamic font locking.

I'd be happy to contribute to some short term fixes, feel free to tag me in any related discussions

Cheers! Write-up coming.

[yuhan0]

What I intend to mean is, a buffer that is running clojure-mode should always map to some ns, not nil. user is a reasonable default.

Hmm, I'm not fully convinced of this, or that EDN data files should map to an arbitrary Clojure namespace for DWIM purposes. Evaluating code from an EDN file seems like an anti-pattern, and formatters should correctly handle the nil case for files that don't have an intrinsic namespace.

Maybe this argues for the existence of a lightweight edn-mode that I've seen suggested before? Analogous to json-mode : js2-mode. Probably a subject for another discussion on the clojure-mode repo.

[bbatsov]

I very much like the idea of a lightweight edn-mode. I rarely use EDN, otherwise I would have done this by now.

…

On Sun, May 21, 2023, at 8:56 AM, yuhan0 wrote: > What I intend to mean is, a buffer that is running clojure-mode should always map to some ns, not nil. user is a reasonable default. > Hmm, I'm not fully convinced of this, or that EDN data files *should* map to an arbitrary Clojure namespace for DWIM purposes. Evaluating code from an EDN file seems like an anti-pattern, and formatters should correctly handle the nil case for files that don't have an intrinsic namespace. Maybe this argues for the existence of a lightweight `edn-mode` that I've seen suggested before? Analogous to json-mode : js2-mode. Probably a subject for another discussion on the clojure-mode repo. — Reply to this email directly, view it on GitHub <#3345 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZLSXPOGXGAHUOS6YE353XHG4BNANCNFSM6AAAAAAYHZB674>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[bbatsov]

@yuhan0 fixed this in #3355

[yuhan0]

Actually only the first (performance related) part of this issue was fixed - as far as I'm aware the second bit about the state handler incorrectly applying font-lock across different unrelated sessions is still unsolved. Just a minor thing to tie in with any discussions re. Sesman and the connection API.

[vemv]

I do plan to work on Sesman very soon. As mentioned we have two foreseen phases [...]

FYI, finally I've decided that there will be only one phase - the "real" one :)

The reason being, I have good availability, so I can just give it one good go, avoiding intermediate states that might only add noise.

I'll create an issue (to be self-assigned).