-
Notifications
You must be signed in to change notification settings - Fork 211
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
Make schema?
, into-schema?
open to extensions
#664
base: master
Are you sure you want to change the base?
Conversation
Sadly But, I think we can have both: first doing the fast instance check and fallback to |
@@ -1958,7 +1958,7 @@ | |||
|
|||
(defn schema? | |||
"Checks if x is a Schema instance" | |||
[x] (#?(:clj instance?, :cljs implements?) malli.core.Schema x)) | |||
[x] (satisfies? Schema x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fast & extendable:
(defn schema?
"Checks if x is a Schema instance"
[x] (or (#?(:clj instance?, :cljs implements?) malli.core.Schema x)) (satisfies? Schema x))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty neat! Thanks.
However this only results for what we can call the "green path" e.g. x
was a malli.core.Schema
.
If x
belonged an unrelated type e.g. 42
, then the cost of both or
branches would be inflicted, for nothing.
One possible fix would be to reshuffle the couple cond
s that use schema?
:
Lines 1974 to 1976 in 68bb521
(schema? ?schema) ?schema | |
(into-schema? ?schema) (-into-schema ?schema nil nil options) | |
(vector? ?schema) (let [v #?(:clj ^IPersistentVector ?schema, :cljs ?schema) |
By placing the schema?
check after the vector?
check, one surely most code paths won't ever hit the satisifes?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option would be to make the extra check dependent on a compile-time condition:
(defmacro maybe-slow-schema-check [x]
(if (System/getProperty "malli.maybe-slow-schema-check")
`(satisfies? Schema ~x)
false))
(defn schema?
"Checks if x is a Schema instance"
[x] (or (#?(:clj instance?, :cljs implements?) malli.core.Schema x)) (maybe-slow-schema-check x))
This would work with my use case since I only use protocol extensions within a specific dev-time task - not as a general assumption in my production codebase.
Of course it's not as general of a solution but it might be a worthwhile alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions.
There is malli.perf.creation-perf-test
which has utilities for testing the perf gains / losses.
This code:
(defn validator []
(m/validator
[:map
[:x boolean?]
[:y {:optional true} int?]
[:z [:map
[:x boolean?]
[:y {:optional true} int?]]]]))
currently calls the schema?
and into-schema?
with the args that don't match the predicate:
"schema?" [:map [:x #object[clojure.core$boolean_QMARK_ 0x53667cbe "clojure.core$boolean_QMARK_@53667cbe"]] [:y {:optional true} #object[clojure.core$int_QMARK_ 0x21ca93f9 "clojure.core$int_QMARK_@21ca93f9"]] [:z [:map [:x #object[clojure.core$boolean_QMARK_ 0x53667cbe "clojure.core$boolean_QMARK_@53667cbe"]] [:y {:optional true} #object[clojure.core$int_QMARK_ 0x21ca93f9 "clojure.core$int_QMARK_@21ca93f9"]]]]]
"into-schema?" [:map [:x #object[clojure.core$boolean_QMARK_ 0x53667cbe "clojure.core$boolean_QMARK_@53667cbe"]] [:y {:optional true} #object[clojure.core$int_QMARK_ 0x21ca93f9 "clojure.core$int_QMARK_@21ca93f9"]] [:z [:map [:x #object[clojure.core$boolean_QMARK_ 0x53667cbe "clojure.core$boolean_QMARK_@53667cbe"]] [:y {:optional true} #object[clojure.core$int_QMARK_ 0x21ca93f9 "clojure.core$int_QMARK_@21ca93f9"]]]]]
"into-schema?" :map
"schema?" #object[clojure.core$boolean_QMARK_ 0x53667cbe "clojure.core$boolean_QMARK_@53667cbe"]
"into-schema?" #object[clojure.core$boolean_QMARK_ 0x53667cbe "clojure.core$boolean_QMARK_@53667cbe"]
"schema?" #IntoSchema{:type boolean?}
"schema?" #object[clojure.core$int_QMARK_ 0x21ca93f9 "clojure.core$int_QMARK_@21ca93f9"]
"into-schema?" #object[clojure.core$int_QMARK_ 0x21ca93f9 "clojure.core$int_QMARK_@21ca93f9"]
"schema?" #IntoSchema{:type int?}
"schema?" [:map [:x #object[clojure.core$boolean_QMARK_ 0x53667cbe "clojure.core$boolean_QMARK_@53667cbe"]] [:y {:optional true} #object[clojure.core$int_QMARK_ 0x21ca93f9 "clojure.core$int_QMARK_@21ca93f9"]]]
"into-schema?" [:map [:x #object[clojure.core$boolean_QMARK_ 0x53667cbe "clojure.core$boolean_QMARK_@53667cbe"]] [:y {:optional true} #object[clojure.core$int_QMARK_ 0x21ca93f9 "clojure.core$int_QMARK_@21ca93f9"]]]
"into-schema?" :map
"schema?" #object[clojure.core$boolean_QMARK_ 0x53667cbe "clojure.core$boolean_QMARK_@53667cbe"]
"into-schema?" #object[clojure.core$boolean_QMARK_ 0x53667cbe "clojure.core$boolean_QMARK_@53667cbe"]
"schema?" #IntoSchema{:type boolean?}
"schema?" #object[clojure.core$int_QMARK_ 0x21ca93f9 "clojure.core$int_QMARK_@21ca93f9"]
"into-schema?" #object[clojure.core$int_QMARK_ 0x21ca93f9 "clojure.core$int_QMARK_@21ca93f9"]
"schema?" #IntoSchema{:type int?}
perf:
(p/bench (validator))
- currently: 1.3µs
- just
satisfies?
: 90µs - or: 90µs
- or +
vector?
check first: 50µs
to make it fast and extendable, we would have implement Schema
and IntoSchema
for the default case and use some tagging methods to actually tell if it satisfies the protocol or not. Sieppari uses that. Malli had an elegant hack for caching the satisifies?
call results, hinted by Alex. Could be re-introduced. Or some other way to write extensions for classes.
lot of work that could be easily fixed in clojure.core
, go vote the issue up :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go vote the issue up
Thumbed!
Hope some sort of fix can make it into Malli in the meantime. Feel free to close this PR if you think the final approach would look too different (I'm not familiar with the tagging technique at all).
For now I'll use some monkey patching but I'd sure love something that felt more dependable.
Hi there,
in Malli 0.8.3 it was easier to implement custom Schema types.
In 0.8.4, protocol extension effectively became a closed thing, since custom implementations would be omitted by
schema?
/into-schema?
, and therefore also omitted by dependent logic.I'd be much thankful if it was kept open, which would allow me to continue experimenting with Var support (which so far has been a success and a justified venture).
In general, other users might have other interesting ideas to implement, so leaving the protocol open doesn't seem too crazy.
I'm aware
satisfies?
has a higher cost thaninstance?
, howeversatisfies?
is far from being implemented naively AFAIK.Cheers - V