Performance and correctness pass on the nREPL message logger#3928
Merged
Conversation
nrepl-log-message wrapped the message in nrepl-plist-put to attach a time-stamp field for display. plist-put extends the list in place when the key is absent, so the original response dict ended up with an extra "time-stamp" key that callbacks downstream of nrepl--dispatch-response would see on a freshly-arrived message. Build a fresh head and cons the time-stamp pair onto a shared tail instead; nothing downstream sees the change.
nrepl-message-buffer-max-size and nrepl-message-buffer-reduce-denominator
were defconst, but the docstrings invite tuning ("defaults to 4",
"limits the number of buffer shrinking operations"). defcustom matches
the docstring intent and lets users adjust through Customize without
re-evaluating constants.
The two non-dict, non-marker branches in nrepl-log-pp-object called pp, which is significantly slower than prin1 and only pays off when output needs multi-line layout. nREPL leaf values are almost always atoms or short lists where prin1 produces the same single-line output. The non-list branch also dropped one of two trailing newlines pp emitted, removing a blank line between entries that was probably never intended.
The old implementation copy-sequence'd the cdr, seq-partition'd into pairs, sorted alphabetically, seq-map'd for name lengths, seq-max'd, seq-filter + seq-remove'd to split off special keys, seq-concatenate'd twice, apply'd seq-concatenate to flatten back into a plist, then cl-loop'd to emit -- roughly six list-sized allocations plus an O(n log n) sort per logged message, half of it just to assemble a plist that immediately got destructured again. Replace with one pass: bucket special keys into a fixed-position vector (so they emit in canonical id/op/session/time-stamp order without sorting), collect the rest in insertion order, and track the widest key inline for column alignment. No more sort, no more copy-sequence, allocations down to a single small vector plus the others list. Behavior change: non-special keys now print in the order they appeared in the dict instead of alphabetically. The order reflects how the peer constructed the message, which is more useful for protocol debugging than alphabetic by accident.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four small follow-ups to #3927 covering performance and correctness in
nrepl-log-messageand friends. The big one is that the logger was mutating the live response dict to attach its display timestamp, so downstream callbacks were seeing a straytime-stampkey on freshly-arrived messages. The rest is a single-pass rewrite ofnrepl-log--pp-listlike(no more sort/copy-sequence/concat pipeline per message),pp→prin1in the leaf paths, and promoting the buffer-size knobs todefcustomto match their docstrings.Behavior change worth flagging: non-special keys now print in insertion order instead of alphabetically. Specials still come first in
id/op/session/time-stamporder.eldev test)eldev lint)