cmcfarlen commented on code in PR #13769:
URL: https://github.com/apache/trafficserver/pull/13769#discussion_r4189253692
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -5112,6 +5112,49 @@
HttpTransact::handle_no_cache_operation_on_forward_server_response(State *s)
return;
}
+// Revalidation merges before #13405 deleted and re-added caching headers on
every 304, and a cache
+// round trip left each dead slot unreclaimable, so a long-lived object's
cached response could grow
+// to hundreds of field blocks. Deleted slots are now reused, but a header's
blocks are never
+// released and every copy and marshal keeps them, so such a header stays
bloated for as long as it
+// is cached and each revalidation rewrites all of it. When it holds well over
the blocks its live
+// fields need, replace it with a copy of just its live fields, in slot order
so field and duplicate
+// order are unchanged. The copy has to land in a fresh heap: copying onto the
existing one frees the
+// dead blocks but leaves their space inside the region that marshal writes to
disk.
+bool
+HttpTransact::compact_cached_response_header(HTTPHdr *cached_header)
+{
+ int blocks = 0;
+
+ for (MIMEFieldBlockImpl const *fblock =
&cached_header->m_mime->m_first_fblock; fblock != nullptr; fblock =
fblock->m_next) {
+ ++blocks;
+ }
+ int const live = cached_header->fields_count();
+
+ if (blocks <= live / MIME_FIELD_BLOCK_SLOTS + 2) {
+ return false;
+ }
+
+ int const before = cached_header->m_heap->marshal_length();
+ HTTPHdr compact;
+
+ compact.create(HTTPType::RESPONSE, cached_header->version_get());
+ compact.status_set(cached_header->status_get());
+ compact.reason_set(cached_header->reason_get());
+ for (auto &field : *cached_header) {
+ // name_get() gives a well-known name its canonical spelling; keep the one
the header carries.
+ MIMEField *copy = compact.field_create(std::string_view{field.m_ptr_name,
field.m_len_name});
+ compact.field_value_set(copy, field.value_get());
+ compact.field_attach(copy);
Review Comment:
Leaving this as is. The rebuilt field prints as `Name: value`, which means
the same thing in HTTP as the origin's raw spacing. Raw formatting is already
lost whenever a string heap is coalesced (`MIMEFieldBlockImpl::move_strings`
clears `m_n_v_raw_printable`, with a FIX ME for exactly this), so it was never
guaranteed for the lifetime of a cached object. This path also only runs for
headers bloated by older builds. Keeping the raw bytes would mean copying each
name, separator and value contiguously into the new heap, which adds complexity
with no functional benefit. I'll reword the description so "byte-identical"
clearly applies to the test's programmatically built header.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -5112,6 +5112,49 @@
HttpTransact::handle_no_cache_operation_on_forward_server_response(State *s)
return;
}
+// Revalidation merges in older builds deleted and re-added caching headers on
every 304, and a cache
+// round trip left each dead slot unreclaimable, so a long-lived object's
cached response could grow
+// to hundreds of field blocks. Deleted slots are now reused, but a header's
blocks are never
+// released and every copy and marshal keeps them, so such a header stays
bloated for as long as it
+// is cached and each revalidation rewrites all of it. When it holds well over
the blocks its live
+// fields need, replace it with a copy of just its live fields, in slot order
so field and duplicate
+// order are unchanged. The copy has to land in a fresh heap: copying onto the
existing one frees the
+// dead blocks but leaves their space inside the region that marshal writes to
disk.
+bool
+HttpTransact::compact_cached_response_header(HTTPHdr *cached_header)
+{
+ int blocks = 0;
+
+ for (MIMEFieldBlockImpl const *fblock =
&cached_header->m_mime->m_first_fblock; fblock != nullptr; fblock =
fblock->m_next) {
+ ++blocks;
+ }
+ int const live = cached_header->fields_count();
+
+ if (blocks <= live / MIME_FIELD_BLOCK_SLOTS + 2) {
+ return false;
+ }
+
+ int const before = cached_header->m_heap->marshal_length();
+ HTTPHdr compact;
+
+ compact.create(HTTPType::RESPONSE, cached_header->version_get());
Review Comment:
Fixed in bf0cd78a74: the empty response is now created without a version,
and the cached version is set with `version_set()`. The test's
`build_response()` had the same `create(RESPONSE, HTTP_1_1)` call, so both
sides were 1.0 and the comparison passed. It now sets the version explicitly
and checks `version_get() == HTTP_1_1` after compaction.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -5112,6 +5112,49 @@
HttpTransact::handle_no_cache_operation_on_forward_server_response(State *s)
return;
}
+// Revalidation merges before #13405 deleted and re-added caching headers on
every 304, and a cache
+// round trip left each dead slot unreclaimable, so a long-lived object's
cached response could grow
+// to hundreds of field blocks. Deleted slots are now reused, but a header's
blocks are never
+// released and every copy and marshal keeps them, so such a header stays
bloated for as long as it
+// is cached and each revalidation rewrites all of it. When it holds well over
the blocks its live
+// fields need, replace it with a copy of just its live fields, in slot order
so field and duplicate
+// order are unchanged. The copy has to land in a fresh heap: copying onto the
existing one frees the
+// dead blocks but leaves their space inside the region that marshal writes to
disk.
+bool
+HttpTransact::compact_cached_response_header(HTTPHdr *cached_header)
+{
+ int blocks = 0;
+
+ for (MIMEFieldBlockImpl const *fblock =
&cached_header->m_mime->m_first_fblock; fblock != nullptr; fblock =
fblock->m_next) {
+ ++blocks;
+ }
+ int const live = cached_header->fields_count();
+
+ if (blocks <= live / MIME_FIELD_BLOCK_SLOTS + 2) {
+ return false;
+ }
Review Comment:
Fixed in bf0cd78a74: the needed block count is now rounded up (minimum one).
Added tests at the boundary: 17 live fields in 4 blocks are left alone, and in
5 blocks they are compacted back to 2.
--
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]
