Factory function symbol caching in durability is not thread safe
Closed this issue · 4 comments
The caching I added in #245 potentially reads and writes to a non-thread-safe map from multiple threads. The purpose of this issue is to make this caching thread safe.
Draft at WilliamParker@dcc0b23
I also considered just synchronizing the map but since the operation you're avoiding is fairly quick anyway and we're just trying to squeeze out maximum performance I'm currently leaning toward just reverting to the non-cached path in case of thread contention.
Repeating the performance test used in
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
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> (time (do (serde records) nil))
"Elapsed time: 54395.25184 msecs"
nil
clara.test-fressian> (time (do (serde records) nil))
"Elapsed time: 48495.305745 msecs"
nil
The performance in a single-threaded test with this locking looks similar to our previous performance.
After some reflection I don't think doing anything more complicated than just synchronizing the map is worth the complication until and unless we have evidence of performance problems from that approach.
PR submitted at #254
Some performance testing with this approach:
user> (set! *warn-on-reflection* true)
true
user> (require 'clara.test-fressian)
nil
user> (in-ns 'clara.test-fressian)
#namespace[clara.test-fressian]
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
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> (time (do (serde records) nil))
"Elapsed time: 48797.011063 msecs"
nil
Closing since the PR has been merged. As discussed there we may revisit this subject later but for now we have corrected the defect.