Improve performance of nrepl-dict-get

[yuhan0]

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):

• [x] The commits are consistent with our contribution guidelines
• [ ] You've added tests (if possible) to cover your change(s)
• [x] All tests are passing (eldev test)
• [x] All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
• [ ] 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.

[bbatsov]

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]

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

[yuhan0]

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]

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]

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]

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

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!