Skip to content

Commit

Permalink
[Fix #1079] Don't font-lock very long results
Browse files Browse the repository at this point in the history 
Font-locking extremely long strings is pretty slow (and might generate
errors in some cases), so it's now conditional. This behavior is
adjustable via `cider-font-lock-max-length`.
  • Loading branch information
@bbatsov
bbatsov committed Aug 9, 2015
1 parent a84e2c1 commit 6d70bcc
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 9 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@

* [#1164](https://github.com/clojure-emacs/cider/pull/1164): Fix an error in `cider-browse-ns--doc-at-point`.
* [#1189](https://github.com/clojure-emacs/cider/issues/1189): Don't show result from automatic ns form evaluation.
* [#1079](https://github.com/clojure-emacs/cider/issues/1079): Don't try to font-lock very long results. The maximum font-lockable result length is controlled by `cider-font-lock-max-length`.

## 0.9.1 / 2015-06-24

Expand Down
23 changes: 14 additions & 9 deletions cider-util.el
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@
(require 'cl-lib)
(require 'clojure-mode)

(defconst cider-font-lock-max-length 10000
"The max length of strings to fontify in `cider-font-lock-as'.")

(defun cider-util--hash-keys (hashtable)
"Return a list of keys in HASHTABLE."
(let ((keys '()))
Expand Down Expand Up @@ -83,15 +86,17 @@ PROP is the name of a text property."

(defun cider-font-lock-as (mode string)
"Use MODE to font-lock the STRING."
(with-temp-buffer
(insert string)
;; suppress major mode hooks as we care only about their font-locking
;; otherwise modes like whitespace-mode and paredit might interfere
(setq-local delay-mode-hooks t)
(setq delayed-mode-hooks nil)
(funcall mode)
(font-lock-ensure)
(buffer-string)))
(if (< (length string) cider-font-lock-max-length)
(with-temp-buffer
(insert string)
;; suppress major mode hooks as we care only about their font-locking
;; otherwise modes like whitespace-mode and paredit might interfere
(setq-local delay-mode-hooks t)
(setq delayed-mode-hooks nil)
(funcall mode)
(font-lock-ensure)
(buffer-string))
string))

(defun cider-font-lock-region-as (mode beg end &optional buffer)
"Use MODE to font-lock text between BEG and END.
Expand Down

5 comments on commit 6d70bcc

@Malabarba
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to #1115?

@bbatsov
Copy link
Member Author

@bbatsov bbatsov commented on 6d70bcc Aug 9, 2015

Choose a reason for hiding this comment

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

Not sure. It's just a theory of mine that it might be related to some extent, but I haven't benchmarked anything. The problem is definitely not 100% font-locking related as the REPL would grind to a halt even before there was Clojure font-locking for results.

@expez
Copy link
Member

@expez expez commented on 6d70bcc Aug 9, 2015

Choose a reason for hiding this comment

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

fwiw I've seen emacs lock up many times over long output, but I've never seen the stackoverflow issue. This leads me to believe they're unrelated and that #1079 might be some weird edgecase instead of a more common occurrence.

@Malabarba
Copy link
Member

Choose a reason for hiding this comment

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

True. I've had the same experience as you, @expez

@bbatsov
Copy link
Member Author

@bbatsov bbatsov commented on 6d70bcc Aug 9, 2015

Choose a reason for hiding this comment

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

I don't really care about the error, I just think font-locking huge strings is probably pretty expensive, so doing a big of optimization might be useful. As I said the original ticket is the oldest in the issue tracker, so this is definitely a problem which existing for a while. I didn't tackle it mostly because I hoped switching to comint would fix it, but it seems this is not going to happen any time soon.

Please sign in to comment.