arturobernalg commented on code in PR #453:
URL:
https://github.com/apache/httpcomponents-client/pull/453#discussion_r1224705130
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheUpdateHandler.java:
##########
@@ -123,18 +128,29 @@ public HttpCacheEntry updateParentCacheEntry(
return new HttpCacheEntry(
src.getRequestInstant(),
src.getResponseInstant(),
+ src.getRequestMethod(),
+ src.getRequestURI(),
+ headers(src.requestHeaderIterator()),
src.getStatus(),
- src.getHeaders(),
+ headers(src.headerIterator()),
resource,
variantMap);
}
- private Header[] mergeHeaders(final HttpCacheEntry entry, final
HttpResponse response) {
- if (DateSupport.isAfter(entry, response, HttpHeaders.DATE)) {
- return entry.getHeaders();
+ private static HeaderGroup headers(final Iterator<Header> it) {
+ final HeaderGroup headerGroup = new HeaderGroup();
+ while (it.hasNext()) {
+ headerGroup.addHeader(it.next());
}
+ return headerGroup;
+ }
+
+ private HeaderGroup mergeHeaders(final HttpCacheEntry entry, final
HttpResponse response) {
Review Comment:
Currently, the method iterates over the response headers twice: once to
remove matching headers from the cache, and once to add the response headers.
In both iterations, the Content-Encoding and Content-Length headers are being
skipped. This seems inefficient.
##########
httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/HttpCacheEntryMatcher.java:
##########
@@ -92,6 +91,41 @@ public boolean matches(final Object item) {
return false;
}
+ private static boolean instantEqual(final Instant expected, final Instant
other) {
+ final Instant expectedMs = expected != null ?
expected.truncatedTo(ChronoUnit.MILLIS) : null;
+ final Instant otherMs = other != null ?
other.truncatedTo(ChronoUnit.MILLIS) : null;
+ return Objects.equals(expectedMs, otherMs);
+ }
+
+ private static boolean headersEqual(final Iterator<Header> expected, final
Iterator<Header> other) {
+ while (expected.hasNext()) {
+ final Header h1 = expected.next();
+ if (!other.hasNext()) {
+ return false;
+ }
+ final Header h2 = other.next();
+ if (!h1.getName().equals(h2.getName()) ||
!Objects.equals(h1.getValue(), h2.getValue())) {
+ return false;
+ }
+ }
+ if (other.hasNext()) {
+ return false;
+ }
+ return true;
+ }
+
+ private static boolean mapEqual(final Map<?, ?> expected, final Map<?, ?>
actual) {
Review Comment:
Your current mapEqual method should work correctly, but I think it can be
optimized a bit.
You can first check if the sizes of the maps are equal. If not, we can
immediately return false without further processing.
Then I would use allMatch function with a lambda expression to compare each
entry in the expected map with its corresponding entry in the actual map. The
beauty of allMatch is that it exhibits short-circuiting behavior: as soon as it
encounters an entry that doesn't meet the provided condition, it stops
processing, which could save us some computation time.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/HttpByteArrayCacheEntrySerializer.java:
##########
@@ -163,57 +125,98 @@ public HttpByteArrayCacheEntrySerializer() {
* <p>
* The metadata is encoded into HTTP pseudo-headers for storage.
*
- * @param httpCacheEntry the HttpCacheStorageEntry to serialize.
+ * @param storageEntry the HttpCacheStorageEntry to serialize.
* @return the byte array containing the serialized HttpCacheStorageEntry.
* @throws ResourceIOException if there is an error during serialization.
*/
@Override
- public byte[] serialize(final HttpCacheStorageEntry httpCacheEntry) throws
ResourceIOException {
- if (httpCacheEntry.getKey() == null) {
- throw new IllegalStateException("Cannot serialize cache object
with null storage key");
- }
- // content doesn't need null-check because it's validated in the
HttpCacheStorageEntry constructor
-
- // Fake HTTP request, required by response generator
- // Use request method from httpCacheEntry, but as far as I can tell it
will only ever return "GET".
- final HttpRequest httpRequest = new
BasicHttpRequest(httpCacheEntry.getContent().getRequestMethod(), "/");
-
- final SimpleHttpResponse httpResponse =
cachedHttpResponseGenerator.generateResponse(httpRequest,
httpCacheEntry.getContent());
- final int size = httpResponse.getHeaders().length +
(httpResponse.getBodyBytes() != null ? httpResponse.getBodyBytes().length : 0);
- try (ByteArrayOutputStream out = new ByteArrayOutputStream(size)) {
- escapeHeaders(httpResponse);
- addMetadataPseudoHeaders(httpResponse, httpCacheEntry);
-
- final byte[] bodyBytes = httpResponse.getBodyBytes();
- int resourceLength = 0;
-
- if (bodyBytes == null) {
- // This means no content, for example a 204 response
- httpResponse.addHeader(SC_HEADER_NAME_NO_CONTENT,
Boolean.TRUE.toString());
- } else {
- resourceLength = bodyBytes.length;
- }
+ public byte[] serialize(final HttpCacheStorageEntry storageEntry) throws
ResourceIOException {
+ final String key = storageEntry.getKey();
+ final HttpCacheEntry cacheEntry = storageEntry.getContent();
+ final Resource resource = cacheEntry.getResource();
+ try (ByteArrayOutputStream out = new ByteArrayOutputStream((resource
!= null ? (int) resource.length() : 0) + DEFAULT_BUFFER_SIZE)) {
final SessionOutputBuffer outputBuffer = new
SessionOutputBufferImpl(bufferSize);
+ final CharArrayBuffer line = new
CharArrayBuffer(DEFAULT_BUFFER_SIZE);
+
+ line.append(HC_CACHE_VERSION_LINE);
+ outputBuffer.writeLine(line, out);
+
+ line.clear();
Review Comment:
This looks duplicate too.
what about something like this
```java
private void writeCacheEntry(final SessionOutputBufferImpl outputBuffer,
final CharArrayBuffer line, final String key, final String value) throws
IOException {
line.clear();
line.append(key);
line.append(": ");
line.append(value);
outputBuffer.writeLine(line, out);
}
```
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/HttpByteArrayCacheEntrySerializer.java:
##########
@@ -230,316 +233,174 @@ public HttpCacheStorageEntry deserialize(final byte[]
serializedObject) throws R
if (serializedObject == null || serializedObject.length == 0) {
throw new ResourceIOException("Serialized object is null or
empty");
}
- try (final InputStream in = new ByteArrayInputStream(serializedObject);
- final ByteArrayOutputStream bytesOut = new
ByteArrayOutputStream(serializedObject.length) // this is bigger than necessary
but will save us from reallocating
- ) {
+ try (final InputStream in = new
ByteArrayInputStream(serializedObject)) {
final SessionInputBufferImpl inputBuffer = new
SessionInputBufferImpl(bufferSize);
- final SimpleHttpResponse response =
responseParser.parse(inputBuffer, in);
-
- if (LOG.isDebugEnabled()) {
- LOG.debug("Deserializing cache entry with headers: {}",
response.getHeaders());
+ final CharArrayBuffer line = new
CharArrayBuffer(DEFAULT_BUFFER_SIZE);
+ checkReadResult(inputBuffer.readLine(line, in));
+ final String versionLine = line.toString();
+ if (!versionLine.equals(HC_CACHE_VERSION_LINE)) {
+ throw new ResourceIOException("Unexpected cache entry version
line");
+ }
+ final List<Header> cacheHeaders = new ArrayList<>();
+ while (true) {
Review Comment:
or, Instead of parsing the headers and populating the cacheHeaders list in
one loop and then processing the headers in another, we can combine these
operations.
This should improve both performance (by avoiding extra iterations) and
memory usage (by avoiding storing an intermediate list of Header objects).
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/HttpByteArrayCacheEntrySerializer.java:
##########
@@ -230,316 +233,174 @@ public HttpCacheStorageEntry deserialize(final byte[]
serializedObject) throws R
if (serializedObject == null || serializedObject.length == 0) {
throw new ResourceIOException("Serialized object is null or
empty");
}
- try (final InputStream in = new ByteArrayInputStream(serializedObject);
- final ByteArrayOutputStream bytesOut = new
ByteArrayOutputStream(serializedObject.length) // this is bigger than necessary
but will save us from reallocating
- ) {
+ try (final InputStream in = new
ByteArrayInputStream(serializedObject)) {
final SessionInputBufferImpl inputBuffer = new
SessionInputBufferImpl(bufferSize);
- final SimpleHttpResponse response =
responseParser.parse(inputBuffer, in);
-
- if (LOG.isDebugEnabled()) {
- LOG.debug("Deserializing cache entry with headers: {}",
response.getHeaders());
+ final CharArrayBuffer line = new
CharArrayBuffer(DEFAULT_BUFFER_SIZE);
+ checkReadResult(inputBuffer.readLine(line, in));
+ final String versionLine = line.toString();
+ if (!versionLine.equals(HC_CACHE_VERSION_LINE)) {
+ throw new ResourceIOException("Unexpected cache entry version
line");
+ }
+ final List<Header> cacheHeaders = new ArrayList<>();
+ while (true) {
Review Comment:
This looks duplication in the block of code where headers are added to
requestHeaders and responseHeaders. Both blocks are performing the same
operations and only differ in the HeaderGroup instance to which they add
headers.
--
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-unsubscr...@hc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org
For additional commands, e-mail: dev-h...@hc.apache.org
