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

Improve performance of nrepl-dict-get #3713

Merged
merged 5 commits into from
Jun 11, 2024

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Jun 11, 2024

See #3711

These changes do not affect the semantics of nrepl-dict-get (covered by existing tests), but byte compilation will raise a warning on any 3-argument usage, hopefully decreasing the risk of its future removal.


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual
extremely useful.

(if (nrepl-dict-contains dict key)
(lax-plist-get (cdr dict) key)
default)
(or (lax-plist-get (cdr dict) key)
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 add some comments here, so someone won't "optimize" away the optimizations in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, and rebased to resolve the conflicts

@bbatsov
Copy link
Member

bbatsov commented Jun 11, 2024

Btw, how about removing such checks:

    (if (nrepl-dict-p dict)...

They obviously have some overhead and seem pointless in practice as we do expect a dict to be pass. I've never been found of so defensive style of programming. (although I do get you're getting nicer errors when you mess up something this way)

@bbatsov
Copy link
Member

bbatsov commented Jun 11, 2024

Ah, I see you've inlined nrepl-dict-p, so I'm guessing it's not as big of a problem now.

Avoid full traversal of dict in 2-argument call
Calling the function with 3 arguments will continue to work, but
raise a warning during byte compilation.
@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 11, 2024

Yeah, I didn't specifically profile the effect of nrepl-dict-p but I'd guess that the overhead from it is negligible anyway.

One other thought I had was whether we could overload all of these functions to accept a hash-table as argument, that should be a non-breaking change and allow us to swap out the implementations of those huge dicts under the hood, similar to how Clojure treats PersistentArrayMap / PersistentHashMap?

Something like

(defun nrepl-dict-get (dict key)
  (cond ((null dict) nil)
        ((nrepl-dict-p dict)
         <<current-implementation>>)
        ((hash-table-p dict)
         (gethash key dict))
        (t
         (error "Not an nREPL dict object: %s" dict))))

@bbatsov bbatsov merged commit 105da31 into clojure-emacs:master Jun 11, 2024
38 of 39 checks passed
@bbatsov
Copy link
Member

bbatsov commented Jun 11, 2024

One other thought I had was whether we could overload all of these functions to accept a hash-table as argument, that should be a non-breaking change and allow us to swap out the implementations of those huge dicts under the hood, similar to how Clojure treats PersistentArrayMap / PersistentHashMap?

I think that's a pretty reasonable idea, as I was thinking something quite similar over the weekend when we started to discuss this. It'd be an easy and fairly transparent change.

@katomuso
Copy link
Contributor

One other thought I had was whether we could overload all of these functions to accept a hash-table as argument, that should be a non-breaking change and allow us to swap out the implementations of those huge dicts under the hood, similar to how Clojure treats PersistentArrayMap / PersistentHashMap?

Sounds really interesting. It will be nice to see some examples of where we can actually use hash tables instead of dicts.

@bbatsov
Copy link
Member

bbatsov commented Jun 11, 2024

@katomuso Well, we can just pick some size after which dicts become hashes (e.g. 10 elements) and this would be done transparently by nrepl-dict constructors. If you check the related discussion you'll see that @yuhan0 found some pretty big dicts in some instances (that are frequently consulted).

@katomuso
Copy link
Contributor

Yeah, I've checked the discussion. I guess it is just hard to wrap my head around how exactly this should work. Will be looking for further updates on the topic!

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