eric-maynard commented on code in PR #490:
URL: https://github.com/apache/polaris/pull/490#discussion_r1964860211
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCache.java:
##########
@@ -71,12 +71,13 @@ public EntityCache(@NotNull PolarisRemoteCache
polarisRemoteCache) {
};
// use a Caffeine cache to purge entries when those have not been used for
a long time.
- // Assuming 1KB per entry, 100K entries is about 100MB.
this.byId =
Caffeine.newBuilder()
- .maximumSize(100_000) // Set maximum size to 100,000 elements
+ .maximumWeight(100 * EntityWeigher.WEIGHT_PER_MB) // Goal is ~100MB
+ .weigher(EntityWeigher.asWeigher())
.expireAfterAccess(1, TimeUnit.HOURS) // Expire entries after 1
hour of no access
.removalListener(removalListener) // Set the removal listener
+ .softValues() // Account for memory pressure
Review Comment:
It's true that a substantial amount of soft references _can cause GC
overhead -- and if you're able to observe that on this branch, I think sharing
those results would be helpful. I did not see such a thing in my testing, but I
didn't try various GC implementations or anything like that.
More importantly, I think this analysis rests on a flawed assumption here:
> memory (allotted for cache
Right now, we allot essentially _infinite_ memory to the cache which is
clearly incorrect. We need to bound that memory footprint, and we should also
provide a mechanism to release that memory when even that footprint causes
memory pressure when combined with everything else running in the JVM.
I do not really think ~100M is sufficient memory to cause issues, but again
if there's some test that shows significant performance degradation due to a
cache of this size doing GC, let's reconsider whether soft values make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
