direct-linking fix: use clojure.core/declare for JVM clojure #487
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I’d like to merge this! As long as there’s no downsides. Can you please run |
|
With changes, at saberstack/datascript@adea735 : % git branch -v
* master adea735 direct-linking fix: use clojure.core/declare for JVM clojure
% ./script/bench_clj.sh
CLJ: add-1 add-5 add-all find-datom find-datoms freeze init pull-many pull-many-entities pull-one pull-one-entities pull-wildcard q1 q2 q3 q4 q5-shortcircuit qpred1 qpred2 retract-5 rules-long-10x3 rules-long-30x3 rules-long-30x5 rules-wide-3x3 rules-wide-4x6 rules-wide-5x3 rules-wide-7x3 thaw
add-1 493.5 ms/op
add-5 957.5 ms/op
add-all 966.6 ms/op
find-datom 0.532 ms/op
find-datoms 0.510 ms/op
freeze 431.4 ms/op
init 17.4 ms/op
pull-many 0.792 ms/op
pull-many-entities 1.9 ms/op
pull-one 0.470 ms/op
pull-one-entities 0.675 ms/op
pull-wildcard 1.9 ms/op
q1 0.585 ms/op
q2 1.6 ms/op
q3 2.3 ms/op
q4 3.2 ms/op
q5-shortcircuit 0.212 ms/op
qpred1 3.7 ms/op
qpred2 6.6 ms/op
retract-5 345.2 ms/op
rules-long-10x3 0.817 ms/op
rules-long-30x3 8.5 ms/op
rules-long-30x5 11.8 ms/op
rules-wide-3x3 0.236 ms/op
rules-wide-4x6 7.6 ms/op
rules-wide-5x3 2.4 ms/op
rules-wide-7x3 35.6 ms/op
thaw 1460.0 ms/opBefore changes, at fa222f7 : % git checkout fa222f7b1b05d4382414022ede011c88f3bad462
HEAD is now at fa222f7 Return lein to CI
raspasov@m2-1 datascript % git branch -v
* (HEAD detached at fa222f7) fa222f7 Return lein to CI
master adea735 direct-linking fix: use clojure.core/declare for JVM clojure
raspasov@m2-1 datascript % ./script/bench_clj.sh
CLJ: add-1 add-5 add-all find-datom find-datoms freeze init pull-many pull-many-entities pull-one pull-one-entities pull-wildcard q1 q2 q3 q4 q5-shortcircuit qpred1 qpred2 retract-5 rules-long-10x3 rules-long-30x3 rules-long-30x5 rules-wide-3x3 rules-wide-4x6 rules-wide-5x3 rules-wide-7x3 thaw
add-1 509.4 ms/op
add-5 992.9 ms/op
add-all 990.6 ms/op
find-datom 0.528 ms/op
find-datoms 0.527 ms/op
freeze 424.3 ms/op
init 17.6 ms/op
pull-many 0.807 ms/op
pull-many-entities 1.9 ms/op
pull-one 0.473 ms/op
pull-one-entities 0.678 ms/op
pull-wildcard 2.0 ms/op
q1 0.605 ms/op
q2 1.6 ms/op
q3 2.3 ms/op
q4 3.2 ms/op
q5-shortcircuit 0.212 ms/op
qpred1 4.1 ms/op
qpred2 6.8 ms/op
retract-5 342.4 ms/op
rules-long-10x3 0.801 ms/op
rules-long-30x3 8.3 ms/op
rules-long-30x5 11.6 ms/op
rules-wide-3x3 0.233 ms/op
rules-wide-4x6 7.6 ms/op
rules-wide-5x3 2.4 ms/op
rules-wide-7x3 35.3 ms/op
thaw 1450.5 ms/op |
|
One more run, with the latest change (two more (declare+ ...) gone from JVM Clojure: TLDR; I don't see a significant difference outside of normal run-to-run variability. % ./script/bench_clj.sh
CLJ: add-1 add-5 add-all find-datom find-datoms freeze init pull-many pull-many-entities pull-one pull-one-entities pull-wildcard q1 q2 q3 q4 q5-shortcircuit qpred1 qpred2 retract-5 rules-long-10x3 rules-long-30x3 rules-long-30x5 rules-wide-3x3 rules-wide-4x6 rules-wide-5x3 rules-wide-7x3 thaw
add-1 503.2 ms/op
add-5 976.2 ms/op
add-all 974.6 ms/op
find-datom 0.539 ms/op
find-datoms 0.517 ms/op
freeze 436.0 ms/op
init 18.0 ms/op
pull-many 0.822 ms/op
pull-many-entities 1.9 ms/op
pull-one 0.495 ms/op
pull-one-entities 0.676 ms/op
pull-wildcard 1.9 ms/op
q1 0.592 ms/op
q2 1.6 ms/op
q3 2.3 ms/op
q4 3.3 ms/op
q5-shortcircuit 0.221 ms/op
qpred1 3.9 ms/op
qpred2 7.2 ms/op
retract-5 352.7 ms/op
rules-long-10x3 0.814 ms/op
rules-long-30x3 8.5 ms/op
rules-long-30x5 11.8 ms/op
rules-wide-3x3 0.235 ms/op
rules-wide-4x6 7.6 ms/op
rules-wide-5x3 2.4 ms/op
rules-wide-7x3 35.6 ms/op
thaw 1449.2 ms/op |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem:
When Direct Linking is enabled, many public functions do not work, in the case below
nth-datomcannot be found – NoSuchMethodError.Here's an inline repro:
clj -Sdeps '{:deps {datascript/datascript {:mvn/version "1.7.4"}}}' -J-Dclojure.compiler.direct-linking=true Clojure 1.12.0 user=> (require '[datascript.core :as d]) nil user=> (nth (d/datom 1 :e "v") 0) Execution error (NoSuchMethodError) at datascript.db.Datom/nth (db.cljc:184). 'java.lang.Object datascript.db$nth_datom.invokeStatic(java.lang.Object, java.lang.Object)' user=>(likely) Root cause:
Certain fns are declared with a custom macro,
(declare+ ...). When direct linking is enabled, those fns are not found.(potential) Fix:
From reading some of the docstrings in
(ns datascript.db)I inferred that(declare+ ...)is only required for ClojureScript. I tried completely bypassing it for Clojure JVM and it seems to work. Please let me know if there are bigger implications that I am not seeing.