Skip to content
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.

Sign up for GitHub

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

Fix performance in fringe overlay placement #1964

Merged
merged 1 commit into from
Apr 3, 2017
Merged

Fix performance in fringe overlay placement #1964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

### Bugs Fixed

* [#1962](https://github.com/clojure-emacs/cider/issues/1962): Fix performance in fringe overlay placement
* [#1947](https://github.com/clojure-emacs/cider/issues/1947): Fix error on `cider-jack-in` when `enlighten-mode` is enabled.
* [#1588](https://github.com/clojure-emacs/cider/issues/1588): Redirect `*err*`, `java.lang.System/out`, and `java.lang.System/err` to REPL buffer on all attached sessions.
* [#1707](https://github.com/clojure-emacs/cider/issues/1707): Allow to customize line truncating in CIDER's special buffers.
Expand Down
8 changes: 6 additions & 2 deletions cider-interaction.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,7 @@ REPL buffer. This is controlled via
(current-buffer))
(save-excursion
(goto-char beg)
(remove-overlays beg end 'cider-fringe-indicator)
(condition-case nil
(while (progn (clojure-forward-logical-sexp)
(and (<= (point) end)
Expand All @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about the purpose of this. How exactly does capturing this value help solve the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just not sure this is the right way to check if there are overlays there or not.

Copy link
Member

@xiongtx xiongtx Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@phillord phillord Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, beg and end are also captured; I just copied what was already there. Regardless, are you happy to accept the PR now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'll take it as is.

Just add a changelog entry about the bugfix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, there's already one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

(nrepl-make-response-handler (or buffer eval-buffer)
(lambda (_buffer value)
(if beg
(cider--make-fringe-overlays-for-region beg end)
(unless fringed
(cider--make-fringe-overlays-for-region beg end)
(setq fringed t))
(cider--make-fringe-overlay end))
(cider--display-interactive-eval-result value end))
(lambda (_buffer out)
Expand Down