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

Replace .entryAt with .valAt during validation #1079

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

alexander-yakushev
Copy link
Contributor

@alexander-yakushev alexander-yakushev commented Jul 25, 2024

Since most implementations of Clojure maps have an allocating .entryAt (because they don't keep keyvalues as ready-to-give-out tuples internally), using this method generates garbage.

user=> (def a {:a 1 :b 2})
#'user/a

;; Note that I had to run this a few times for allocations to start reporting,
;; apparently the escape analysis manages to rewrite this sometimes.

user=> (time+ (.entryAt ^clojure.lang.Associative a :a))
Time per call: 5 ns   Alloc per call: 32b   Iterations: 391311081

;; Here's a better demo:

user=> (time+ 1 (crit/quick-bench (.entryAt ^clojure.lang.Associative a :a)))
Evaluation count : 90876120 in 6 samples of 15146020 calls.
             Execution time mean : 4.839308 ns
...
Time per call: 6.31 s   Alloc per call: 26,772,768,824b   Iterations: 1

user=> (time+ 1 (crit/quick-bench (.valAt ^clojure.lang.Associative a :a nil)))
Evaluation count : 109404672 in 6 samples of 18234112 calls.
             Execution time mean : 3.710991 ns
...
Time per call: 6.61 s   Alloc per call: 10,654,080b   Iterations: 1

Since both callsites that use .entryAt don't really need the tuple, just have to know if the key was present in the map, here's a proposal to replace .entryAt with 2-arity .valAt.

@ikitommi ikitommi merged commit 7fd3fd7 into metosin:master Jul 25, 2024
9 checks passed
@ikitommi
Copy link
Member

Good stuff, thanks!

@frenchy64
Copy link
Collaborator

Could we use a private value for not-found? IMO it's an antipattern to use a keyword.

https://frenchy64.github.io/fully-satisfies/latest/io.github.frenchy64.fully-satisfies.uniform.html

@alexander-yakushev
Copy link
Contributor Author

alexander-yakushev commented Jul 26, 2024

It was the easiest to do (and fastest too – using a global private var would involve an extra var dereference fwiw). Does it cause any problems this way?

https://frenchy64.github.io/fully-satisfies/latest/io.github.frenchy64.fully-satisfies.uniform.html

Could you please explain how this namespace and this change are related?

@frenchy64
Copy link
Collaborator

This is the problem.

  (m/validate [:map [:x :any]] {:x ::m/not-found})
  ;=> false

Here's a unit test that demonstrates the same issue with a clojure core fn.

https://github.com/frenchy64/fully-satisfies/blob/516b193ae28178b0ed1303f2596615352ef2044b/test/io/github/frenchy64/fully_satisfies/uniform_test.clj#L10-L16

@alexander-yakushev
Copy link
Contributor Author

alexander-yakushev commented Jul 26, 2024

Sure, I understand that, but is it a problem in practice? The common wisdom is to not defined keywords in namespaces you don't own, so as long as malli.core doesn't use ::not-found keyword anywhere else, there would not be a conflict.

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