Fix weak-ref caching - #649
Conversation
47eeabe to
b3a2b15
Compare
51c3ea7 to
74103a1
Compare
| // All heap references should always be weak-referred to be collectable under memory pressure. | ||
| // The partial tuples should use only weak-references, they are very unhandy anyway and should be avoided. | ||
| private var tuplesByStorage = AtomicMap<Int64, AtomicMap<TupleNumber, WeakRef<Tuple>>>() | ||
| private var tuplesByStorage = AtomicMap<Int64, AtomicMap<TupleNumber, SoftReference<Tuple>>>() |
There was a problem hiding this comment.
I agree that Soft-References improve the situation, but they are not the final solution. Removing the TODO is not appropriate. The comment was still right, we should somehow control cache size. Having some form of minimal size is still the best solution. Even while there no real solution yet, it does not mean the current one is a good final one.
Please add the TODO back into, we should not use WeakReferences, because they will be collected at ones and go always all together, but we should not stick to only soft-references as well. The best is a mechanism that somehow uses a certain amount of strong references, a bunch of soft-references, and the rest weak-references. However, we need to write a better strategy guide, this is a long term goal.
So, for now, I'm totally fine with the switch from Weak- to Soft-References, but not with removing the TODO.
There was a problem hiding this comment.
The comment is still correct for Soft-References, they can as well be selected at any time. It is just less likely.
| val tupleNumber = write.tupleNumber | ||
| tupleNumbers[write.i] = tupleNumber | ||
| val tuple = write.tuple | ||
| if (tuple != null) tuplesByIndex[write.i] = tuple |
There was a problem hiding this comment.
This is a waste of CPU and memory. We have writes, which is a MutableList that is already in order, and every write gets the Tuple attached, why would we want to make another array in the exact same order containing the exact same objects?
P.S.: Be ware that pgWrites is a copy of writes, but sorted by collection, while writes is still the originally ordered list. So tuplesByIndex[write.i] == writes[write.i].tuple.
| featureTuples.setCapacity(writes.size) | ||
| for (i in 0 until writes.size) { | ||
| val tupleNumber = tupleNumbers[i] ?: continue | ||
| val tuple = tuplesByIndex[i] |
There was a problem hiding this comment.
This can be replaced with writes[i].tuple, there is totally no need for yet another array.
There was a problem hiding this comment.
Why was the comment removed, it is still fully correct?
| forceGc() | ||
|
|
||
| val cached = Naksha.cache[tn] | ||
| assertNotNull(cached, "a write-warmed tuple must survive GC via SoftReference (the weak-only tier dropped it)") |
There was a problem hiding this comment.
This is wrong, there is no guarantee for a Soft-Reference to survive a GC. It is just unlikely that it gets collected. Creating a deterministic test on a (defined) none-deterministic behavior is not acceptable! This may work 99% of times, but I do not want it to fail in CICD for some odd reason, just because exactly there GC pressure is so high that the cache is collected.
Signed-off-by: kkin-here <284318677+kkin-here@users.noreply.github.com>
Signed-off-by: kkin-here <284318677+kkin-here@users.noreply.github.com>
74103a1 to
418e2a9
Compare
Code Coverage
|
2 participants
No description provided.