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

chore: remove unnecessary binding of current message #736

Conversation

hugoduncan
Copy link
Member

The binding is not used within the body expressions, and costs a thread
local binding, producing garbage. This changes ensures print-stream
produces no memory garbage.


Before submitting a PR make sure the following things have been done:

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings
  • You've updated the README (if adding/changing middleware)

Note: If you're just starting out to hack on cider-nrepl you might find
nREPL's documentation and the
"Design" section of the README extremely useful.*

Thanks!

The binding is not used within the body expressions, and costs a thread
local binding, producing garbage.  This changes ensures print-stream
produces no memory garbage.
@vemv
Copy link
Member

vemv commented Dec 9, 2021

The binding is not used within the body expressions

Perhaps while this is true, the binding is used by the underlying nrepl machinery?

Reading the code, there seems to be intentful work for binding msg to a precise thing out of a msg-seq. i.e. if something bothered to do that, probably it was for a reason?

If we couldn't determine the original intent, perhaps I'd default to not changing the code - a binding's runtime cost surely is marginal in the context of printing stuff to *out* (i.e. IO-heavy workload).

Each printed line might also imply transient garbage, so in terms of memory, the binding's cost also seems marginal.

@bbatsov
Copy link
Member

bbatsov commented Dec 9, 2021

If the output is still properly redirected it's fine to remove this.

@bbatsov
Copy link
Member

bbatsov commented Dec 9, 2021

I did notice that this code has been there since the first commit of the file. I'm guessing one reason to have it is to avoid the original msg that triggered some output being replaced by a subsequent evaluation.

@hugoduncan
Copy link
Member Author

My understanding is:

The forking-printer just takes values and forwards them. There is no execution of user code, so it can not possibly use the message in any.

print-stream invokes the forking printer flush every 100ms, so the current code produces garbage every 100ms.

@hugoduncan
Copy link
Member Author

hugoduncan commented Dec 9, 2021

I think my previous patch introduced a bug - the outout to original-output should happen outside the loop created by with-out-binding. #737

@bbatsov bbatsov merged commit 9dc1b38 into clojure-emacs:master Dec 10, 2021
@bbatsov
Copy link
Member

bbatsov commented Dec 10, 2021

Okay, you've convinced me. :-) Thanks!

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.

3 participants