Welcome! Please see the About page for a little more info on how this works.

+12 votes
in Protocols by
Currently `satisfies?` doesn't use the same impl cache used by protocol methods, making it too slow for real world usage.

With:

(defprotocol p (f [_]))
(deftype x [])
(deftype y [])
(extend-type x p (f [_]))


Before patch:

(let [s "abc"] (bench (instance? CharSequence s))) ;; Execution time mean : 1.358360 ns
(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 112.649568 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 2.605426 µs


*Cause:* `satisfies?` calls `find-protocol-impl` to see whether an object implements a protocol, which checks for whether x is an instance of the protocol interface or whether x's class is one of the protocol implementations (or if its in an inheritance chain that would make this true). This check is fairly expensive and not cached.

*Proposed:* Extend the protocol's method impl cache to also handle (and cache) instance checks (including negative results).

After patch:

(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 79.321426 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 77.410858 ns


*Patch*: CLJ-1814-v7.patch (depends on CLJ-2426)

25 Answers

0 votes
by

Comment made by: bronsa

That's not good, I'll take a look later today, thanks

0 votes
by

Comment made by: bronsa

This was an issue with how nil was cached, I decided to special-case nil to skip the method cache, removing the need for all the NIL funny business and fixing this bad interaction between false and nil.

0 votes
by

Comment made by: michaelblume

Not sure if it's in scope for this ticket, but given that this wasn't caught, there should probably be more protocol dispatch tests

0 votes
by

Comment made by: alexmiller

yes, should definitely add

0 votes
by

Comment made by: michaelblume

Patch with test

0 votes
by

Comment made by: michaelblume

Verified that test fails with v4 patch:

`

 [java] Testing clojure.test-clojure.protocols
 [java]
 [java] FAIL in (test-default-dispatch) (protocols.clj:695)
 [java] expected: (= "Object impl" (test-dispatch false))
 [java]   actual: (not (= "Object impl" "Nil impl"))
 [java]
 [java] FAIL in (test-default-dispatch) (protocols.clj:695)
 [java] expected: (= "Object impl" (test-dispatch true))
 [java]   actual: (not (= "Object impl" "Nil impl"))

`

0 votes
by

Comment made by: michaelblume

Has this patch missed the 1.9 train? There was a fix we were hoping to make in HoneySQL that I'd hesitate to make with satisfies? as slow as it is.

0 votes
by

Comment made by: alexmiller

Not necessarily. We don't add features after 1.9 but perf stuff like this is still possible. It's been vetted by Rich. It's in my list of stuff to screen.

0 votes
by

Comment made by: bronsa

Updated patch to work after the new instance-based protocol dispatch, this ticket should wait for CLJ-2426 first

0 votes
by
Reference: https://clojure.atlassian.net/browse/CLJ-1814 (reported by bronsa)
...