You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "ok2c (via GitHub)" <gi...@apache.org> on 2023/06/26 10:02:05 UTC
[GitHub] [httpcomponents-client] ok2c opened a new pull request, #464: HTTPCLIENT-2277: optimization of internal cache operations
ok2c opened a new pull request, #464:
URL: https://github.com/apache/httpcomponents-client/pull/464
This is a complete redesign and re-write of the internal cache implementations intended
* to improve internal cache APIs
* to provide support for partial matches (no direct match but there are variants for the same root)
* to eliminate repetitive, redundant cache key calculations
* to eliminate unnecessary duplicate cache look-ups in the course of a single message exchange
@arturobernalg Please review.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #464: HTTPCLIENT-2277: optimization of internal cache operations
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #464:
URL: https://github.com/apache/httpcomponents-client/pull/464#discussion_r1244819488
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -165,9 +167,11 @@ public ClassicHttpResponse execute(
setResponseStatus(context, CacheResponseStatus.CACHE_MODULE_RESPONSE);
return new BasicClassicHttpResponse(HttpStatus.SC_NOT_IMPLEMENTED);
}
- final HttpCacheEntry entry = responseCache.getCacheEntry(target, request);
+ final CacheMatch result = responseCache.match(target, request);
+ final CacheHit hit = result != null ? result.hit : null;
+ final CacheHit root = result != null ? result.root : null;
- final SimpleHttpResponse fatalErrorResponse = getFatallyNonCompliantResponse(request, context, entry != null);
+ final SimpleHttpResponse fatalErrorResponse = getFatallyNonCompliantResponse(request, context, hit != null);
Review Comment:
@arturobernalg This is a matter of taste. I find `hit != null` clearer but it is just me.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on a diff in pull request #464: HTTPCLIENT-2277: optimization of internal cache operations
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #464:
URL: https://github.com/apache/httpcomponents-client/pull/464#discussion_r1244244483
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -165,9 +167,11 @@ public ClassicHttpResponse execute(
setResponseStatus(context, CacheResponseStatus.CACHE_MODULE_RESPONSE);
return new BasicClassicHttpResponse(HttpStatus.SC_NOT_IMPLEMENTED);
}
- final HttpCacheEntry entry = responseCache.getCacheEntry(target, request);
+ final CacheMatch result = responseCache.match(target, request);
+ final CacheHit hit = result != null ? result.hit : null;
+ final CacheHit root = result != null ? result.root : null;
- final SimpleHttpResponse fatalErrorResponse = getFatallyNonCompliantResponse(request, context, entry != null);
+ final SimpleHttpResponse fatalErrorResponse = getFatallyNonCompliantResponse(request, context, hit != null);
Review Comment:
I've noticed that the `hit != null` condition is being checked numerous times in the code. To enhance code readability and reduce redundancy, I suggest encapsulating this logic within the `CacheMatch` class itself. We could add a `hasHit()` method as follows:
```
public boolean hasHit() {
return hit != null;
}
```
This way, we can simply call `cacheMatch.hasHit()` wherever we need to check if hit is not null, improving the code's readability and maintainability."
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheMatch.java:
##########
@@ -26,30 +26,26 @@
*/
package org.apache.hc.client5.http.impl.cache;
-import org.apache.hc.client5.http.cache.HttpCacheEntry;
-
-/** Records a set of information describing a cached variant. */
-class Variant {
-
- private final String cacheKey;
- private final HttpCacheEntry entry;
-
- public Variant(final String cacheKey, final HttpCacheEntry entry) {
- this.cacheKey = cacheKey;
- this.entry = entry;
- }
+/**
+ * Represents a full match or a partial match (variant options)
+ * result of a cache lookup operation.
+ */
+class CacheMatch {
- public String getCacheKey() {
- return cacheKey;
- }
+ final CacheHit hit;
Review Comment:
@ok2c
Shouldn't this class be fully immutable?
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheHit.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.impl.cache;
+
+import org.apache.hc.client5.http.cache.HttpCacheEntry;
+
+class CacheHit {
+
+ final String rootKey;
+ final String variantKey;
+ final HttpCacheEntry entry;
+
+ public CacheHit(final String rootKey, final String variantKey, final HttpCacheEntry entry) {
Review Comment:
Shouldn't this class be fully immutable? Also, shouldn't `rootKey` and `entry` be null-safe?
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java:
##########
@@ -498,22 +502,24 @@ public void cancelled() {
storeRequestIfModifiedSinceFor304Response(request, backendResponse);
} else {
LOG.debug("Backend response is not cacheable");
- responseCache.flushCacheEntriesFor(target, request, new FutureCallback<Boolean>() {
+ if (!Method.isSafe(request.getMethod())) {
Review Comment:
Perhaps we could consider delegating this check (`!Method.isSafe(request.getMethod())`) inside the method of the class `HttpAsyncCache` and `BasicHttpCache` itself. This way, the method becomes more resilient and we ensure the check is always performed when needed, reducing the potential for errors.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -507,29 +522,26 @@ ClassicHttpResponse negotiateResponseFromVariants(
}
final String resultEtag = resultEtagHeader.getValue();
- final Variant matchingVariant = variants.get(resultEtag);
- if (matchingVariant == null) {
+ final CacheHit match = variantMap.get(resultEtag);
+ if (match == null) {
LOG.debug("304 response did not contain ETag matching one sent in If-None-Match");
return callBackend(target, request, scope, chain);
}
- final HttpCacheEntry variantEntry = matchingVariant.getEntry();
-
- if (revalidationResponseIsTooOld(backendResponse, variantEntry)
+ if (revalidationResponseIsTooOld(backendResponse, match.entry)
&& (request.getEntity() == null || request.getEntity().isRepeatable())) {
final ClassicHttpRequest unconditional = conditionalRequestBuilder.buildUnconditionalRequest(request);
return callBackend(target, unconditional, scope, chain);
}
recordCacheUpdate(scope.clientContext);
- final HttpCacheEntry responseEntry = responseCache.updateVariantEntry(
- target, conditionalRequest, backendResponse, variantEntry, requestDate, responseDate);
- if (shouldSendNotModifiedResponse(request, responseEntry)) {
- return convert(responseGenerator.generateNotModifiedResponse(responseEntry), scope);
+ final CacheHit hit = responseCache.update(match, backendResponse, requestDate, responseDate);
+ if (shouldSendNotModifiedResponse(request, hit.entry)) {
+ return convert(responseGenerator.generateNotModifiedResponse(hit.entry), scope);
}
- final SimpleHttpResponse response = responseGenerator.generateResponse(request, responseEntry);
- responseCache.reuseVariantEntryFor(target, request, backendResponse, responseEntry, requestDate, responseDate);
+ final SimpleHttpResponse response = responseGenerator.generateResponse(request, hit.entry);
+ responseCache.storeReusing(hit, target, request, backendResponse, requestDate, responseDate);
Review Comment:
Shouldn't be the `entry` instead of the `hit`?
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -346,20 +350,19 @@ ClassicHttpResponse revalidateCacheEntry(
}
if (statusCode == HttpStatus.SC_NOT_MODIFIED) {
- final HttpCacheEntry updatedEntry = responseCache.updateEntry(
- target, request, cacheEntry, backendResponse, requestDate, responseDate);
+ final CacheHit updated = responseCache.update(hit, request, backendResponse, requestDate, responseDate);
if (suitabilityChecker.isConditional(request)
- && suitabilityChecker.allConditionalsMatch(request, updatedEntry, Instant.now())) {
- return convert(responseGenerator.generateNotModifiedResponse(updatedEntry), scope);
+ && suitabilityChecker.allConditionalsMatch(request, updated.entry, Instant.now())) {
+ return convert(responseGenerator.generateNotModifiedResponse(updated.entry), scope);
}
- return convert(responseGenerator.generateResponse(request, updatedEntry), scope);
+ return convert(responseGenerator.generateResponse(request, updated.entry), scope);
}
if (staleIfErrorAppliesTo(statusCode)
- && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, cacheEntry, getCurrentDate())
- && validityPolicy.mayReturnStaleIfError(requestCacheControl, responseCacheControl, cacheEntry, responseDate)) {
+ && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit, getCurrentDate())
Review Comment:
I noticed in the `staleResponseNotAllowed` method we are passing in the complete `CacheHit` object, but only the `entry` attribute within it is being used. To maintain the original structure would it be feasible to pass just the `entry` object instead?
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #464: HTTPCLIENT-2277: optimization of internal cache operations
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #464:
URL: https://github.com/apache/httpcomponents-client/pull/464#issuecomment-1610967076
@arturobernalg Please do another pass.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c merged pull request #464: HTTPCLIENT-2277: optimization of internal cache operations
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c merged PR #464:
URL: https://github.com/apache/httpcomponents-client/pull/464
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #464: HTTPCLIENT-2277: optimization of internal cache operations
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #464:
URL: https://github.com/apache/httpcomponents-client/pull/464#discussion_r1244845040
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheHit.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.impl.cache;
+
+import org.apache.hc.client5.http.cache.HttpCacheEntry;
+
+class CacheHit {
+
+ final String rootKey;
+ final String variantKey;
+ final HttpCacheEntry entry;
+
+ public CacheHit(final String rootKey, final String variantKey, final HttpCacheEntry entry) {
Review Comment:
@arturobernalg The class is immutable. It is also internal so I forwent getters and null checks for its instance variables.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -507,29 +522,26 @@ ClassicHttpResponse negotiateResponseFromVariants(
}
final String resultEtag = resultEtagHeader.getValue();
- final Variant matchingVariant = variants.get(resultEtag);
- if (matchingVariant == null) {
+ final CacheHit match = variantMap.get(resultEtag);
+ if (match == null) {
LOG.debug("304 response did not contain ETag matching one sent in If-None-Match");
return callBackend(target, request, scope, chain);
}
- final HttpCacheEntry variantEntry = matchingVariant.getEntry();
-
- if (revalidationResponseIsTooOld(backendResponse, variantEntry)
+ if (revalidationResponseIsTooOld(backendResponse, match.entry)
&& (request.getEntity() == null || request.getEntity().isRepeatable())) {
final ClassicHttpRequest unconditional = conditionalRequestBuilder.buildUnconditionalRequest(request);
return callBackend(target, unconditional, scope, chain);
}
recordCacheUpdate(scope.clientContext);
- final HttpCacheEntry responseEntry = responseCache.updateVariantEntry(
- target, conditionalRequest, backendResponse, variantEntry, requestDate, responseDate);
- if (shouldSendNotModifiedResponse(request, responseEntry)) {
- return convert(responseGenerator.generateNotModifiedResponse(responseEntry), scope);
+ final CacheHit hit = responseCache.update(match, backendResponse, requestDate, responseDate);
+ if (shouldSendNotModifiedResponse(request, hit.entry)) {
+ return convert(responseGenerator.generateNotModifiedResponse(hit.entry), scope);
}
- final SimpleHttpResponse response = responseGenerator.generateResponse(request, responseEntry);
- responseCache.reuseVariantEntryFor(target, request, backendResponse, responseEntry, requestDate, responseDate);
+ final SimpleHttpResponse response = responseGenerator.generateResponse(request, hit.entry);
+ responseCache.storeReusing(hit, target, request, backendResponse, requestDate, responseDate);
Review Comment:
@arturobernalg Done.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #464: HTTPCLIENT-2277: optimization of internal cache operations
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #464:
URL: https://github.com/apache/httpcomponents-client/pull/464#discussion_r1244842939
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -346,20 +350,19 @@ ClassicHttpResponse revalidateCacheEntry(
}
if (statusCode == HttpStatus.SC_NOT_MODIFIED) {
- final HttpCacheEntry updatedEntry = responseCache.updateEntry(
- target, request, cacheEntry, backendResponse, requestDate, responseDate);
+ final CacheHit updated = responseCache.update(hit, request, backendResponse, requestDate, responseDate);
if (suitabilityChecker.isConditional(request)
- && suitabilityChecker.allConditionalsMatch(request, updatedEntry, Instant.now())) {
- return convert(responseGenerator.generateNotModifiedResponse(updatedEntry), scope);
+ && suitabilityChecker.allConditionalsMatch(request, updated.entry, Instant.now())) {
+ return convert(responseGenerator.generateNotModifiedResponse(updated.entry), scope);
}
- return convert(responseGenerator.generateResponse(request, updatedEntry), scope);
+ return convert(responseGenerator.generateResponse(request, updated.entry), scope);
}
if (staleIfErrorAppliesTo(statusCode)
- && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, cacheEntry, getCurrentDate())
- && validityPolicy.mayReturnStaleIfError(requestCacheControl, responseCacheControl, cacheEntry, responseDate)) {
+ && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit, getCurrentDate())
Review Comment:
@arturobernalg Done.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #464: HTTPCLIENT-2277: optimization of internal cache operations
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #464:
URL: https://github.com/apache/httpcomponents-client/pull/464#discussion_r1244852899
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java:
##########
@@ -498,22 +502,24 @@ public void cancelled() {
storeRequestIfModifiedSinceFor304Response(request, backendResponse);
} else {
LOG.debug("Backend response is not cacheable");
- responseCache.flushCacheEntriesFor(target, request, new FutureCallback<Boolean>() {
+ if (!Method.isSafe(request.getMethod())) {
Review Comment:
@arturobernalg I am not sure. I do not think this is a cache storage layer decision whether a method is considered safe or not. But let's discuss it when I finish refactoring the cache eviction logic.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #464: HTTPCLIENT-2277: optimization of internal cache operations
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #464:
URL: https://github.com/apache/httpcomponents-client/pull/464#discussion_r1244817938
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheMatch.java:
##########
@@ -26,30 +26,26 @@
*/
package org.apache.hc.client5.http.impl.cache;
-import org.apache.hc.client5.http.cache.HttpCacheEntry;
-
-/** Records a set of information describing a cached variant. */
-class Variant {
-
- private final String cacheKey;
- private final HttpCacheEntry entry;
-
- public Variant(final String cacheKey, final HttpCacheEntry entry) {
- this.cacheKey = cacheKey;
- this.entry = entry;
- }
+/**
+ * Represents a full match or a partial match (variant options)
+ * result of a cache lookup operation.
+ */
+class CacheMatch {
- public String getCacheKey() {
- return cacheKey;
- }
+ final CacheHit hit;
Review Comment:
@arturobernalg It is. All its instance variables are final and immutable. There are no access methods but that does not make the class mutable.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org