-
Notifications
You must be signed in to change notification settings - Fork 938
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
HTTPCLIENT-2277: Revision and simplification of the cache eviction logic #466
Conversation
Sure, just I’ll need time |
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.
@ok2c The change-set looks good. Just keep in mind my comment.
} | ||
|
||
@Override | ||
public Cancellable flushCacheEntriesInvalidatedByExchange( | ||
public Cancellable evictInvalidated( |
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.
What about renaming to ´evictInvalidatedCacheEntries´ for enhanced readability and better clarity of the method's operation on cache entries. This makes it clear that the method is operating on cache entries specifically, not just any arbitrary objects or data.
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.
@arturobernalg Renamed to #evictInvalidatedEntries
.
@@ -129,6 +130,16 @@ HttpCacheEntry getInternal(final String cacheKey) { | |||
} | |||
} | |||
|
|||
private void removeInternal(final String cacheKey) { |
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.
Shouldn't we use the same name ('evictEntry')? After all, both are internal operations.
removeInternal(rootKey); | ||
if (root.isVariantRoot()) { | ||
for (final String variantKey : root.getVariantMap().values()) { | ||
LOG.debug("Evicting variant cache entry {}", variantKey); |
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.
Consider wrapping all debug log statements with a ´if (LOG.isDebugEnabled())´ check for performance enhancement by avoiding unnecessary string concatenation when debug logging is not enabled.
Also consider replacing the for loop that evicts variant cache entries one by one with a new ´removeEntriesBulk´ method, which will allow us to eliminate multiple entries at once.
public synchronized void removeEntriesBulk(final Collection<String> urls) throws ResourceIOException {
entries.removeEntriesBulk(urls);
}
and
public void removeEntriesBulk(final Collection<String> keysToRemove)
{
this.entrySet().removeIf(entry -> keysToRemove.contains(entry.getKey()));
}
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.
Consider wrapping all debug log statements with a ´if (LOG.isDebugEnabled())´ check for performance enhancement by avoiding unnecessary string concatenation when debug logging is not enabled.
There's no string concatenation/formatting happening with parametrized messages when the target log level is not enabled:
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.
My suggestion was a general recommendation and aimed more at reducing log noise when evicting entries.
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.
@stanio True, there is no string concatenation or formatting but in some cases there can be an extra array allocation (yes, I am aware of overloaded methods that take a fixed number of parameters instead of an array). I think @arturobernalg comment is warranted.
evictEntry(rootKey); | ||
if (root.isVariantRoot()) { | ||
for (final String variantKey : root.getVariantMap().values()) { | ||
LOG.debug("Evicting variant cache entry {}", variantKey); |
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.
Consider wrapping all debug log statements with a ´if (LOG.isDebugEnabled())´ check for performance enhancement by avoiding unnecessary string concatenation when debug logging is not enabled
return cacheInvalidator.flushCacheEntriesInvalidatedByExchange(host, request, response, cacheKeyGenerator, storage, callback); | ||
final int status = response.getCode(); | ||
if (status >= HttpStatus.SC_SUCCESS && status < HttpStatus.SC_CLIENT_ERROR && | ||
!Method.isSafe(request.getMethod())) { |
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.
Given the relatively higher computational cost of Method.isSafe(), it's suggested to move this condition to the beginning of our if-statement for more efficient short-circuiting, which could improve overall performance.
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.
@arturobernalg Should not it be the other way around, cheapest conditions first, most expensive last?
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.
@ok2c Apologies for the confusion caused by my previous comment. Upon further reflection, it's clear that the order of conditions in the if-statement is already well-optimized for short-circuiting.
…gic; conformance to RFC 9111 section 4.4
This change-set revises and simplified the cache eviction logic to ensure conformance with RFC 9111 section 4.4
@arturobernalg Please review.