-
Notifications
You must be signed in to change notification settings - Fork 110
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
Clara durability should prefer ThreadLocal to dynamic vars #251
Clara durability should prefer ThreadLocal to dynamic vars #251
Conversation
@@ -56,3 +56,22 @@ | |||
(persistent!))) | |||
:cljs | |||
(def tuned-group-by clojure.core/group-by)) | |||
|
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.
This is a cljc file, and this is not inside a reader conditional, so this will be compiled in the Clojure and ClojureScript code. ClojureScript is limited to using macros written in Clojure that take unevaluated ClojureScript code and return other ClojureScript code without any embedded JVM or JavaScript objects on either side. I'm surprised that this compiles and I'd like to understand how. In any event though it would make no sense for ClojureScript to use a JVM ThreadLocal so I think this should be inside a clj-only reader conditional.
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.
moved thread-local-binding macro inside clj-only reader conditional
abf2454
to
e156755
Compare
@kulkarnipushkar please squash this PR into one commit. |
a83062b
to
378a7f8
Compare
Elaborating a bit on the motivation for these changes, looking up a Clojure dynamic var involves more work than looking up a single JVM ThreadLocal. The implementation can be found at [1]. Among other things, there is a map of the bindings for the thread that the particular dynamic var involved has to be looked up in. This more dynamic approach allows Clojure to duplicate bindings between threads when a Clojure construct retrieves a new thread from Clojure's thread pool; the heart of this is [2] and its usages. In this case we don't need these properties since our durability implementation doesn't create new threads. Note that user code can be multithreaded; this doesn't impact that. The performance difference between a dynamic var lookup and a ThreadLocal was noticeable in profiling of our production use cases. |
The use of the dyanamic vars in clara.rules.durability and clara.rules.durability.fressian namsespace are wasting unnecessary cpu cycles. A ThreadLocal does a subset of the work done by a Clojure dynamic var and we do not really need any of the more sophisticated properties of dynamic vars for our purposes here. The proposal is to change these to ThreadLocal.
With this change we could see the performance gain, please look at the below test result.
We could also see noticible performance gain in our production workflows.