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

Make schema?, into-schema? open to extensions #664

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vemv
Copy link

@vemv vemv commented Mar 16, 2022

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 than instance?, however satisfies? is far from being implemented naively AFAIK.

Cheers - V

@ikitommi
Copy link
Member

Sadly satisfies? is really slow, 20-30x slower than the Instance check and don't want to take the penalty. See #513 for more details on the perf stuff.

But, I think we can have both: first doing the fast instance check and fallback to satisfies?. Could you change the code to do that?

@@ -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))
Copy link
Member

@ikitommi ikitommi Mar 20, 2022

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

Copy link
Author

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 conds that use schema?:

malli/src/malli/core.cljc

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?.

Copy link
Author

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.

Copy link
Member

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

Copy link
Author

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.

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.

2 participants