Fix performance in fringe overlay placement #1964
Conversation
cider-interaction.el
Outdated
| (nrepl-make-response-handler (or buffer eval-buffer) | ||
| (lambda (_buffer value) | ||
| (if beg | ||
| (cider--make-fringe-overlays-for-region beg end) | ||
| (when (not fringed) |
|
This needs a changelog entry. |
Multiple identical fringe overlays were being placed by overlays were being added to the entire region for every interactive eval in that region. Add check in cider-interactive-eval-handler, so only first value adds fringe overlays. Remove all fringe overlays in region before placing new ones. Addresses clojure-emacs#1962
phillord
force-pushed
the
fix/eval-on-region-performance
branch
from
8993e11 to
db6bd3d
Compare
|
Done |
| @@ -709,11 +710,14 @@ or it can be a list with (START END) of the evaluated region." | |||
| (beg (car-safe place)) | |||
| (end (or (car-safe (cdr-safe place)) place)) | |||
| (beg (when beg (copy-marker beg))) | |||
| (end (when end (copy-marker end)))) | |||
| (end (when end (copy-marker end))) | |||
| (fringed nil)) | |||
There was a problem hiding this comment.
I'm a bit confused about the purpose of this. How exactly does capturing this value help solve the problem?
There was a problem hiding this comment.
--make-fringe-overlays-for-region was getting called multiple times; afaict, the first handler is when ever a value returns right? And this will happen multiple times, if there are multiple top-level forms in the region? But beg and end never change for the life time of the handler, so this makes no sense So, all I am doing here is making sure that fringe-overlays-for-region is only called once in the lifetime of the response handler.
Maybe I am misunderstanding how the response-handler works. AFAICT, nothing with the fringe overlays checks whether the overlay is already there, which is causing the problem.
There was a problem hiding this comment.
I'm just not sure this is the right way to check if there are overlays there or not.
There was a problem hiding this comment.
I'd expect functions that create overlays to be idempotent instead of relying on flags like fringed. That is, if cider--make-fringe-overlays-for-region is called a second time on the same region, it detects that the overlay already exists and does nothing.
You can detect the existence of overlays with functions like overlays-in.
There was a problem hiding this comment.
@xiongtx I think that is a good idea, and the other hunk of the commit tries to address this (belts and braces). But, even here there is a performance issue. make-fringe-overlays makes many overlays, not just one. So, at a minimum, you have to move through every top-level sexp and check for an overlay at each point.
There was a problem hiding this comment.
Nothing from me. I'd simply prefer a solution that doesn't rely on a closure-captured variable as such code would be easier to comprehend.
There was a problem hiding this comment.
Well, beg and end are also captured; I just copied what was already there. Regardless, are you happy to accept the PR now?
There was a problem hiding this comment.
Yeah, I'll take it as is.
Just add a changelog entry about the bugfix.
Multiple identical fringe overlays were being placed by overlays were being
added to the entire region for every interactive eval in that region.
Add check in cider-interactive-eval-handler, so only first value adds fringe
overlays. Remove all fringe overlays in region before placing new ones.
Addresses #1962
Replace this placeholder text with a summary of the changes in your PR.
The more detailed you are, the better.
Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):
make test)M-x checkdocwarningsThanks!
If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.