Cache construction of factory function symbols in Fressian durability implementation
Closed this issue · 3 comments
The function clara.rules.durability.fressian/record-map-constructor-name [1] takes a Clojure record instance and, using the name of its class, performs various string manipulation steps to construct the symbol of its map->RecordNameHere factory function created by the defrecord macro. Examples of the usage of these factory functions can be found at [2]. These operations are fast in absolute terms, but this can be a hotspot when serializing large sessions. A cache mapping a class to the symbol that should be returned should be more performant than executing this work again on every call.
I think we can use WeakHashMap here so that we don't force the JVM to hold onto classes that are otherwise garbage-collectable. Once the class is garbage collectable we shouldn't ever need to look that exact class up again, so I think a weak reference is preferable to a soft reference here. Note that for current purposes if the class is reloaded it is a new class even if there were no functional changes relative to a previously loaded version. My understanding of the difference is that objects only referred to by weak references are garbage-collected aggressively, while objects referred to by soft references are garbage collected only when there is actual memory pressure. If the record class is still reachable I wouldn't expect the factory function for it to be garbage-collectable, so we can just use the default strong reference for the values. Note that the values are symbols to look up the factory function, not the factory function objects themselves, and do not contain a reference to the record Class object, but the namespace must have a strong reference to the symbol in order for it to be able to look up the function. [3] If a value (in the sense of a key-value pair) did have such a reference to a map key we would need to use a WeakReference or a SoftReference to hold that value as discussed in the WeakHashMap JavaDoc.
Pull request created at #246
Some performance testing from this:
;; Create multiple record classes to resist JVM inlining
clara.test-fressian> (defrecord MyRecord1 [n])
clara.test_fressian.MyRecord1
clara.test-fressian> (defrecord MyRecord2 [n])
clara.test_fressian.MyRecord2
clara.test-fressian> (defrecord MyRecord3 [n])
clara.test_fressian.MyRecord3
clara.test-fressian> (defrecord MyRecord4 [n])
clara.test_fressian.MyRecord4
clara.test-fressian> (defrecord MyRecord5 [n])
clara.test_fressian.MyRecord5
;; Put all the records in a vector to force realization and avoid performance impacts from realizing the lazy seq created by for
clara.test-fressian> (def records (into []
cat
(for [_ (range 1e6)]
[(MyRecord1. n) (MyRecord2. n) (MyRecord3. n) (MyRecord4. n) (MyRecord5. n)])))
CompilerException java.lang.RuntimeException: Unable to resolve symbol: n in this context, compiling:(*cider-repl localhost*:127:44)
clara.test-fressian> (def records (into []
cat
(for [n (range 1e6)]
[(MyRecord1. n) (MyRecord2. n) (MyRecord3. n) (MyRecord4. n) (MyRecord5. n)])))
#'clara.test-fressian/records
clara.test-fressian> (count records)
5000000
;; With the current implementation
clara.test-fressian> (time (do (serde records) nil))
"Elapsed time: 140660.824077 msecs"
nil
;; With the implementation in this PR
clara.test-fressian> (require 'clara.test-fressian :reload-all)
nil
clara.test-fressian> (time (do (serde records) nil))
"Elapsed time: 52807.449386 msecs"
nil
Keeping in mind that those numbers are for serialization and deserialization, not just deserialization, this looks like a substantial performance improvement.
+1 to the PR other than my minor comment.
This issue has been resolved by my merging of #246. Closing.