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

Jump to bottom

Use lower nREPL print quotas for interactive evaluation #3635

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use lower nREPL print quotas for interactive evaluation #3635

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

### Changes

- [#3633](https://github.com/clojure-emacs/cider/issues/3633): Use lower nREPL [print quotas](https://nrepl.org/nrepl/1.1/design/middleware.html#pretty-printing) for interactive evaluation.
- This can improve responsiveness for large outputs, with no tradeoffs.
- [#3626](https://github.com/clojure-emacs/cider/issues/3626): `cider-ns-refresh`: jump to the relevant file/line on errors.
- [#3628](https://github.com/clojure-emacs/cider/issues/3628): `cider-ns-refresh`: summarize errors as an overlay.
- Bump the injected nREPL to [1.1.1](https://github.com/nrepl/nrepl/blob/v1.1.1/CHANGELOG.md#111-2024-02-20).
Expand Down
28 changes: 28 additions & 0 deletions cider-client.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,34 @@ is included in the request if non-nil."
(when cider-print-quota
`(("nrepl.middleware.print/quota" ,cider-print-quota))))))

(defun cider--make-interactive-quota ()
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably name this cider--calculate-interactive-print-quota.

"Returns a value `nrepl.middleware.print/quota',
that is suitable for interactive evaluation.

These should be lower than `cider-print-quota',
which is a good default for other use cases
\(repls, logs, stacktraces, tests, etc),
because by definition large values don't fit in the screen
and can slow down performance."
(min (max (round (* (frame-width) (frame-height) 1.5))
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 struggling to understand the logic of those calculations.

Copy link
Member

Choose a reason for hiding this comment

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

Seems pretty discernable to me – the limit is 1.5 full frames of text (one full frame and then some extra on the side). This can be said in a comment nearby.

10000)
30000))

(defun cider--nrepl-interactive-pr-request-map ()
"Map to merge into requests that do not require pretty printing.

Like `cider--nrepl-pr-request-map', but has a more apt
`nrepl.middleware.print/quota' for interactive evaluation."
(let ((cider-print-quota (cider--make-interactive-quota)))
(cider--nrepl-pr-request-map)))

(defun cider--nrepl-interactive-print-request-map (&optional right-margin)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you should use the naming print and pprint in both helpers, as I find the current names confusing.

Copy link
Member

Choose a reason for hiding this comment

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

And change cider--nrepl-pr-request-map's name to match.

Copy link
Member

Choose a reason for hiding this comment

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

The pr one is so-named because it always uses clojure.core/pr, regardless of your printing options. The print one uses whatever your cider-print-fn is set to (which might also be pr, and so will not necessarily pretty print).

"Map to merge into requests that require pretty-printing.
RIGHT-MARGIN specifies the maximum column-width of the printed result, and
is included in the request if non-nil."
(let ((cider-print-quota (cider--make-interactive-quota)))
(cider--nrepl-print-request-map right-margin)))

(defun cider--nrepl-content-type-map ()
"Map to be merged into an eval request to make it use content-types."
'(("content-type" "true")))
Expand Down
32 changes: 16 additions & 16 deletions cider-eval.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,7 +1266,7 @@ arguments and only proceed with evaluation if it returns nil."
(cider-interactive-eval nil
nil
(list start end)
(cider--nrepl-pr-request-map)))
(cider--nrepl-interactive-pr-request-map)))

(defun cider-eval-last-sexp (&optional output-to-current-buffer)
"Evaluate the expression preceding point.
Expand All @@ -1276,7 +1276,7 @@ buffer."
(cider-interactive-eval nil
(when output-to-current-buffer (cider-eval-print-handler))
(cider-last-sexp 'bounds)
(cider--nrepl-pr-request-map)))
(cider--nrepl-interactive-pr-request-map)))

(defun cider-eval-last-sexp-and-replace ()
"Evaluate the expression preceding point and replace it with its result."
Expand All @@ -1291,7 +1291,7 @@ buffer."
(cider-interactive-eval last-sexp
(cider-eval-print-handler)
nil
(cider--nrepl-pr-request-map))))
(cider--nrepl-interactive-pr-request-map))))
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking, this one is not merely a "representational" printing since the result will substitute the code that was evaluated in the buffer. If we truncate that one, user will get incorrect code inserted into their source file (and it will break Paredit too 🤢).

It's not apparent what would be the best UX. Perhaps, doing no replacement and showing the message that the evaluation result is too big to be inserted into the code; but inserting a truncated result is probably not on that list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, yes, as mentioned in OP these defuns deserve being reviewed one by one.

Perhaps, it's doing no replacement and showing the message that the evaluation result is too big to be inserted into the code

If it's > 1MB (our default quota as of master) that would seem reasonable to me. Maybe preview its first lines and ask for a y/n confirmation.

Either way, this is good to simply exclude from the PR for now.


(defun cider-eval-list-at-point (&optional output-to-current-buffer)
"Evaluate the list (eg. a function call, surrounded by parens) around point.
Expand All @@ -1318,7 +1318,7 @@ buffer."
(cider-interactive-eval tapped-form
(when output-to-current-buffer (cider-eval-print-handler))
nil
(cider--nrepl-pr-request-map))))
(cider--nrepl-interactive-pr-request-map))))

(defun cider-tap-sexp-at-point (&optional output-to-current-buffer)
"Evaluate and tap the expression around point.
Expand Down Expand Up @@ -1367,7 +1367,7 @@ When GUESS is non-nil, attempt to extract the context from parent let-bindings."
(cider-interactive-eval code
nil
bounds
(cider--nrepl-pr-request-map))))
(cider--nrepl-interactive-pr-request-map))))

(defun cider-eval-last-sexp-in-context (guess)
"Evaluate the preceding sexp in user-supplied context.
Expand Down Expand Up @@ -1406,7 +1406,7 @@ With the prefix arg INSERT-BEFORE, insert before the form, otherwise afterwards.
(set-marker (make-marker) insertion-point)
cider-comment-prefix)
bounds
(cider--nrepl-pr-request-map))))
(cider--nrepl-interactive-pr-request-map))))

(defun cider-pprint-form-to-comment (form-fn insert-before)
"Evaluate the form selected by FORM-FN and insert result as comment.
Expand Down Expand Up @@ -1436,7 +1436,7 @@ If INSERT-BEFORE is non-nil, insert before the form, otherwise afterwards."
cider-comment-continued-prefix
comment-postfix)
bounds
(cider--nrepl-print-request-map fill-column))))
(cider--nrepl-interactive-print-request-map fill-column))))

(defun cider-pprint-eval-last-sexp-to-comment (&optional insert-before)
"Evaluate the last sexp and insert result as comment.
Expand Down Expand Up @@ -1490,13 +1490,13 @@ honoring SWITCH-TO-REPL, REQUEST-MAP."
"Evaluate the expression preceding point and insert its result in the REPL.
If invoked with a PREFIX argument, switch to the REPL buffer."
(interactive "P")
(cider--eval-last-sexp-to-repl prefix (cider--nrepl-pr-request-map)))
(cider--eval-last-sexp-to-repl prefix (cider--nrepl-interactive-pr-request-map)))

(defun cider-pprint-eval-last-sexp-to-repl (&optional prefix)
"Evaluate expr before point and insert its pretty-printed result in the REPL.
If invoked with a PREFIX argument, switch to the REPL buffer."
(interactive "P")
(cider--eval-last-sexp-to-repl prefix (cider--nrepl-print-request-map fill-column)))
(cider--eval-last-sexp-to-repl prefix (cider--nrepl-interactive-print-request-map fill-column)))
Copy link
Member

Choose a reason for hiding this comment

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

Regarding this and the preceding one: REPL is usually a bit more sturdy to large strings being printed, and the REPL can be scrolled, so a limit of 1 or 1.5 frame is not as clear cut for the REPL. It's true that Emacs still doesn't like huge one-liners and would lag barely/somewhat/terribly on it; but users might still expect that the REPL would handle a large value, and that they can C-a to the beginning and copy the vlaue to kill ring, for example.

That still wouldn't work for values over 1MB as we already truncate those in the REPL too, but between 10k and 1M there is a big area that somebody can be accustomed to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounding reasonable.

I distinctly remember my repl getting slower before the 1MB limit saving the day. Even then, it was an unpleasant experience and has wished the limit came in earlier.

Perhaps we can have a slightly more generous limit e.g. 5 'screenfuls'.

...There will always being some tension between a responsive repl, and being opinionated.

I prefer to be opinionated (i.e. nudge users to use spit if they want a huge result to be grabbable) so that we can reasonably guarantee that the repl is generally always usable.

Copy link
Member

Choose a reason for hiding this comment

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

Kinda shows that SLIME with its clickable buttons in the REPL did one thing right 🤔.

Copy link
Member

Choose a reason for hiding this comment

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

I distinctly remember my repl getting slower before the 1MB limit saving the day. Even then, it was an unpleasant experience and has wished the limit came in earlier.

I tried printing values into the REPL that go over 1MB limit, and while they make Emacs lag slightly when moving the cursor across the REPL, there wasn't anything like Emacs-freezing-100%-cpu-disaster that I remember happening a long time ago. Can you reproduce something like that on your setup? That can help pick up the quota limit better.


(defun cider-eval-print-last-sexp (&optional pretty-print)
"Evaluate the expression preceding point.
Expand All @@ -1507,8 +1507,8 @@ With an optional PRETTY-PRINT prefix it pretty-prints the result."
(cider-eval-print-handler)
(cider-last-sexp 'bounds)
(if pretty-print
(cider--nrepl-print-request-map fill-column)
(cider--nrepl-pr-request-map))))
(cider--nrepl-interactive-print-request-map fill-column)
(cider--nrepl-interactive-pr-request-map))))

(defun cider--pprint-eval-form (form)
"Pretty print FORM in popup buffer."
Expand All @@ -1519,7 +1519,7 @@ With an optional PRETTY-PRINT prefix it pretty-prints the result."
(cider-interactive-eval (when (stringp form) form)
handler
(when (consp form) form)
(cider--nrepl-print-request-map fill-column)))))
(cider--nrepl-interactive-print-request-map fill-column)))))

(defun cider-pprint-eval-last-sexp (&optional output-to-current-buffer)
"Evaluate the sexp preceding point and pprint its value.
Expand Down Expand Up @@ -1583,7 +1583,7 @@ command `cider-debug-defun-at-point'."
(concat "#dbg\n" (cider-defun-at-point)))
nil
(cider-defun-at-point 'bounds)
(cider--nrepl-pr-request-map))))
(cider--nrepl-interactive-pr-request-map))))

(defun cider--insert-closing-delimiters (code)
"Closes all open parenthesized or bracketed expressions of CODE."
Expand Down Expand Up @@ -1615,7 +1615,7 @@ buffer. It constructs an expression to eval in the following manner:
(when output-to-current-buffer
(cider-eval-print-handler))
(list beg-of-defun (point))
(cider--nrepl-pr-request-map))))
(cider--nrepl-interactive-pr-request-map))))

(defun cider--matching-delimiter (delimiter)
"Get the matching (opening/closing) delimiter for DELIMITER."
Expand Down Expand Up @@ -1646,7 +1646,7 @@ buffer. It constructs an expression to eval in the following manner:
(when output-to-current-buffer
(cider-eval-print-handler))
(list beg-of-sexp (point))
(cider--nrepl-pr-request-map))))
(cider--nrepl-interactive-pr-request-map))))

(defun cider-pprint-eval-defun-at-point (&optional output-to-current-buffer)
"Evaluate the \"top-level\" form at point and pprint its value.
Expand Down Expand Up @@ -1685,7 +1685,7 @@ If VALUE is non-nil, it is inserted into the minibuffer as initial input."
(cider-interactive-eval form
nil
nil
(cider--nrepl-pr-request-map))))))
(cider--nrepl-interactive-pr-request-map))))))

(defun cider-read-and-eval-defun-at-point ()
"Insert the toplevel form at point in the minibuffer and output its result.
Expand Down