Copilot commented on code in PR #13769:
URL: https://github.com/apache/trafficserver/pull/13769#discussion_r4156556268
##########
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:
This threshold undercounts the blocks required whenever `live` is not a
multiple of 16. For example, 17 live fields need two blocks, but this treats
one as the baseline and compacts a four-block header even though it has only
two excess blocks; that contradicts the PR's “more than two blocks beyond what
live fields need” threshold and causes avoidable rebuilds. Compute the ceiling
(with one inline block for an empty header) before applying the allowance.
##########
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:
`field_value_set` clears `m_n_v_raw_printable` (`MIME.cc:2137-2138`), while
fields parsed from origin responses normally retain their raw line
representation (`MIME.cc:2555-2564,2622-2624`). Consequently, compaction
changes an unchanged cached line such as `X:\tvalue` to `X: value`; the test
misses this because `build_response` creates fields programmatically with the
raw flag already clear. Preserve each live field's raw bytes/metadata when
rebuilding, and cover a parsed response so compaction remains byte-transparent
as intended.
--
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]
