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

Use lower nREPL print quotas for interactive evaluation #3635

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

Conversation

vemv
Copy link
Member

@vemv vemv commented Mar 14, 2024

Fixes #3633

Seems to work nicely (per the nrepl logs, and evaluating some huge objects).

Marking the PR as draft as I'll want to test out the defuns one by one.

Although many are affected, a good variety of defuns are unchanged - we still use the larger quota for repls, stacktraces, logs, tests, etc - basically whenever the nREPL output is printed to a buffer (vs. to an overlay).

I based the quota size on the frame, as definitionally one cannot display an overlay larger than the current frame.

Cheers - V

@vemv vemv requested a review from bbatsov March 14, 2024 17:18
@bbatsov
Copy link
Member

bbatsov commented Mar 14, 2024

Let me think about this one for a while. I get your point, but I'm wondering if that's the right solution to the problem - this quota exists mostly to avoid out of memory errors on the nREPL server side, for limiting printing there might be other options we can consider.

@vemv
Copy link
Member Author

vemv commented Mar 14, 2024

this quota exists mostly to avoid out of memory errors on the nREPL server sid

TIL tbh!

I quite often see the limit trimming the REPL output, so I thought that was big part of its reason to exist. Or at least it's a fortunate side-effect.

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

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

@bbatsov
Copy link
Member

bbatsov commented Mar 15, 2024

I couple of general concerns?

  • How will the trimmed results look exactly? (both in the minibuffer and in the overlay) I believe we use the same handler for both, so at the very least it should be clear that something got trimmed and what caused this to happen.
  • By trimming results at the nREPL level it won't be possible for people to request the full result to be rendered
  • I'm not sure if the interactive results are streamed (I think not, even if we discussed it with @yuhan0 in the past, if I remember correctly), so perhaps we can make the streamable and if the first chunk is full we can just assume something is relatively big and display it in a buffer instead of an overlay.

I'm also wondering how big of a problem is this in practice as people have rarely complained about it.

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

@bbatsov
Copy link
Member

bbatsov commented Apr 21, 2024

Ping :-)

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.

Reduce the nrepl.middleware.print/quota for interactive eval
2 participants