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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/malli/core.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1907,7 +1907,7 @@

(defn into-schema?
"Checks if x is a IntoSchema instance"
[x] (#?(:clj instance?, :cljs implements?) malli.core.IntoSchema x))
[x] (satisfies? IntoSchema x))

(defn into-schema
"Creates a Schema instance out of type, optional properties map and children"
Expand Down Expand Up @@ -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 •
edited
Loading

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.


(defn schema
"Creates a Schema object from any of the following:
Expand Down
12 changes: 12 additions & 0 deletions test/malli/core_test.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2659,3 +2659,15 @@
(is (m/schema? (via-ast :my/bigger-than-3)))
(is (m/schema? (via-ast :my-bigger-than-4)))
(is (m/schema? (via-ast 'my/bigger-than-5))))))

(defrecord ExtensionExample []
m/Schema
m/IntoSchema)

(deftest schema?-test
(is (m/schema? (reify m/Schema)))
(is (m/schema? (map->ExtensionExample {}))))

(deftest into-schema?-test
(is (m/into-schema? (reify m/IntoSchema)))
(is (m/into-schema? (map->ExtensionExample {}))))