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.

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

Suppress eldoc when the current sexp seems to be too large #1502

Merged
merged 1 commit into from
Jan 4, 2016

Conversation

rfkm
Copy link
Contributor

@rfkm rfkm commented Jan 4, 2016

This PR restricts the range of searching the beginning of the current sexps for eldoc.

Current version of eldoc could be slow in a large buffer because cider-eldoc-beginning-of-sexp could traverse the whole buffer.
Such a situation is especially prone to happen in the REPL buffer.

In the following example, if the cursor is located at the right of the prompt,
cider-eldoc-beginning-of-sexp calls (forward-sexp -1) about 15 times (user> -> line -> a -> is -> ...).

This is a line
This is a line
This is a line
user>

When the REPL buffer has 4k lines and 35k sexp-like objects (this means (forward-sexp -1) will be called 35k times),
cider-eldoc-beginning-of-sexp takes about 4 seconds in my environment.
With my fix, it takes less than 0.001 seconds.

This might be related to #1361.

@@ -107,14 +111,18 @@ Return the number of nested sexp the point was over or after."
(let ((p (point)))
(forward-sexp -1)
(when (< (point) p)
(setq num-skipped-sexps (1+ num-skipped-sexps))))))
(setq num-skipped-sexps (unless (and cider-eldoc-max-num-sexps-to-skip
Copy link
Member

Choose a reason for hiding this comment

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

There should probably be comments here explaining why this is necessary (for posterity's sake).

@bbatsov
Copy link
Member

bbatsov commented Jan 4, 2016

This seems like a good solution for now, although long-term it seems to me that we should improve the process of sexp boundaries detection in the REPL.

@rfkm
Copy link
Contributor Author

rfkm commented Jan 4, 2016

Updated.

Yeah, I had been going to do so with narrowing at first, but I found it difficult to detect the boundary strictly because we might sometimes want eldoc to work with sexp in a log message in a REPL buffer.

bbatsov added a commit that referenced this pull request Jan 4, 2016
Suppress eldoc when the current sexp seems to be too large
@bbatsov bbatsov merged commit debbb94 into clojure-emacs:master Jan 4, 2016
@bbatsov
Copy link
Member

bbatsov commented Jan 4, 2016

because we might sometimes want eldoc to work with sexp in a log message in a REPL buffer.

I don't know - it seems to me that if something isn't REPL input we can safely ignore it in terms of eldoc.

@cap10morgan
Copy link
Contributor

Do we need to add something to the changelog for this? Never mind, should have looked at the changes first. I was expecting to see something in Bug Fixes, but this is perf, so makes sense. I'll get it into the v0.10 PR.

@Malabarba
Copy link
Member

I think the proper way to do this would be to narrow from input-start to end-of-buffer, and then find the start of current sexp by running syntax-ppss. If point is before input-start then just don't do eldoc (or maybe do it by narrowing from beginning of line).

This might still be slow if the user pastes some huge sexp into the REPL (unlikely) but we can get around that by checking the size of the narrowed buffer.

@Malabarba
Copy link
Member

But, in any case, thanks for looking into this. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants