Make `satisfies?` as fast as a protocol method call

Question

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)

Answer 1

Comment made by: bronsa

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

Answer 2

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.

Answer 3

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

Answer 4

Comment made by: alexmiller

yes, should definitely add

Answer 5

Comment made by: michaelblume

Patch with test

Answer 6

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

`

Answer 7

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.

Answer 8

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.

Answer 9

Comment made by: bronsa

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

Answer 10

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