You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bryancall (via GitHub)" <gi...@apache.org> on 2026/01/05 22:27:19 UTC

[PR] Optimize cache miss path with deferred cache write [trafficserver]

bryancall opened a new pull request, #12782:
URL: https://github.com/apache/trafficserver/pull/12782

   Add configuration option `proxy.config.http.cache.defer_write_on_miss` to defer opening the cache for write until after response headers are received and evaluated for cacheability.
   
   ## Performance Impact
   
   When enabled, this optimization:
   - Avoids unnecessary cache overhead for non-cacheable responses
   - Can improve throughput ~21x for non-cacheable content (e.g., `Cache-Control: no-store` responses)
   
   ## Trade-offs
   
   When enabled:
   - May affect read-while-write functionality
   - May reduce request coalescing for popular uncached URLs
   - Recommended only for traffic patterns with mostly unique URLs or predominantly non-cacheable content
   
   ## Configuration
   
   ```yaml
   proxy:
     config:
       http:
         cache:
           defer_write_on_miss: 1  # 0=disabled, 1=enabled
   ```
   
   ## How it works
   
   1. On cache miss, set `cache_open_write_deferred=true` instead of immediately opening cache for write
   2. Go to origin and fetch response headers
   3. Check if response is cacheable:
      - If not cacheable (no-store, etc.): skip cache write entirely
      - If cacheable: open cache for write and proceed normally
   
   **Note:** Default is currently enabled for CI testing. Will be changed to disabled before merge.


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#issuecomment-3893450466

   autest 0


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#discussion_r3011963123


##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3333,6 +3333,62 @@ HttpTransact::handle_cache_write_lock_go_to_origin_continue(State *s)
   }
 }
 
+///////////////////////////////////////////////////////////////////////////////
+// Name       : handle_deferred_cache_write_complete
+// Description: Called after cache write opens for deferred cache write.
+//              We already have the server response headers, so we continue
+//              with cache write handling.
+//
+// Possible Next States From Here:
+// - handle_cache_operation_on_forward_server_response (if cache write succeeded)
+// - handle_no_cache_operation_on_forward_server_response (if cache write failed)
+//
+///////////////////////////////////////////////////////////////////////////////
+void
+HttpTransact::handle_deferred_cache_write_complete(State *s)
+{
+  TxnDbg(dbg_ctl_http_trans, "handle_deferred_cache_write_complete");
+  TxnDbg(dbg_ctl_http_seq, "Entering handle_deferred_cache_write_complete");
+
+  ink_assert(s->cache_info.action == CacheAction_t::PREPARE_TO_WRITE);
+
+  switch (s->cache_info.write_lock_state) {
+  case CacheWriteLock_t::SUCCESS:
+    // Cache write lock acquired successfully
+    TxnDbg(dbg_ctl_http_trans, "[hdcwc] cache write lock acquired");
+    SET_UNPREPARE_CACHE_ACTION(s->cache_info);
+    // Now we have WRITE action, handle the response with caching
+    handle_cache_operation_on_forward_server_response(s);
+    return;
+
+  case CacheWriteLock_t::FAIL:
+    // Could not get cache write lock, continue without caching
+    TxnDbg(dbg_ctl_http_trans, "[hdcwc] cache write lock failed, continuing without cache");
+    Metrics::Counter::increment(http_rsb.cache_open_write_fail_count);
+    s->cache_info.action       = CacheAction_t::NO_ACTION;
+    s->cache_info.write_status = CacheWriteStatus_t::LOCK_MISS;
+    break;
+
+  case CacheWriteLock_t::READ_RETRY:
+    // This shouldn't happen for deferred write since we don't have an object to read

Review Comment:
   If this shouldn't happen, should it be a InkAssert() at least ? Not sure that TxnDbg is the right choice here either, maybe a notice or something else ? 



-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#issuecomment-3715425667

   rerun autest


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#issuecomment-3898944240

   [approve ci]


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#discussion_r2676505893


##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3231,6 +3231,62 @@ HttpTransact::handle_cache_write_lock(State *s)
   }
 }
 
+///////////////////////////////////////////////////////////////////////////////
+// Name       : handle_deferred_cache_write_complete
+// Description: Called after cache write opens for deferred cache write.
+//              We already have the server response headers, so we continue
+//              with cache write handling.
+//
+// Possible Next States From Here:
+// - handle_cache_operation_on_forward_server_response (if cache write succeeded)
+// - handle_no_cache_operation_on_forward_server_response (if cache write failed)
+//
+///////////////////////////////////////////////////////////////////////////////
+void
+HttpTransact::handle_deferred_cache_write_complete(State *s)
+{
+  TxnDbg(dbg_ctl_http_trans, "handle_deferred_cache_write_complete");
+  TxnDbg(dbg_ctl_http_seq, "Entering handle_deferred_cache_write_complete");
+
+  ink_assert(s->cache_info.action == CacheAction_t::PREPARE_TO_WRITE);
+
+  switch (s->cache_info.write_lock_state) {
+  case CacheWriteLock_t::SUCCESS:
+    // Cache write lock acquired successfully
+    TxnDbg(dbg_ctl_http_trans, "[hdcwc] cache write lock acquired");
+    SET_UNPREPARE_CACHE_ACTION(s->cache_info);
+    // Now we have WRITE action, handle the response with caching
+    handle_cache_operation_on_forward_server_response(s);
+    return;
+
+  case CacheWriteLock_t::FAIL:
+    // Could not get cache write lock, continue without caching
+    TxnDbg(dbg_ctl_http_trans, "[hdcwc] cache write lock failed, continuing without cache");
+    Metrics::Counter::increment(http_rsb.cache_open_write_fail_count);
+    s->cache_info.action       = CacheAction_t::NO_ACTION;
+    s->cache_info.write_status = CacheWriteStatus_t::LOCK_MISS;
+    break;
+
+  case CacheWriteLock_t::READ_RETRY:
+    // This shouldn't happen for deferred write since we don't have an object to read
+    TxnDbg(dbg_ctl_http_trans, "[hdcwc] unexpected READ_RETRY for deferred write");
+    s->cache_info.action       = CacheAction_t::NO_ACTION;
+    s->cache_info.write_status = CacheWriteStatus_t::LOCK_MISS;
+    break;
+
+  case CacheWriteLock_t::INIT:
+  default:
+    ink_release_assert(0);
+    break;
+  }
+
+  // If we get here, cache write failed - continue without caching
+  // The original handle_no_cache_operation_on_forward_server_response will be called
+  // but we need to skip the deferred cache write check since we just tried it
+  // s->cache_open_write_deferred is already false from the first call

Review Comment:
   This comment is misleading. The flag `cache_open_write_deferred` was set to false at line 4751 when the cache write was initiated, not "from the first call". Consider revising to: "s->cache_open_write_deferred was already set to false before initiating the cache write" for clarity.
   ```suggestion
     // s->cache_open_write_deferred was already set to false before initiating the cache write
   ```



-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#issuecomment-4828414131

   This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#discussion_r3010332345


##########
configs/records.yaml.default.in:
##########
@@ -86,6 +86,12 @@ records:
 # https://docs.trafficserver.apache.org/en/latest/admin-guide/files/records.yaml.en.html#proxy-config-http-cache-when-to-revalidate
       when_to_revalidate: 0
 
+# Defer cache open write until response headers are received.

Review Comment:
   Instead of this long comment, shouldn't it just link to the relevant docs anchor?



##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3333,6 +3333,62 @@ HttpTransact::handle_cache_write_lock_go_to_origin_continue(State *s)
   }
 }
 
+///////////////////////////////////////////////////////////////////////////////
+// Name       : handle_deferred_cache_write_complete
+// Description: Called after cache write opens for deferred cache write.
+//              We already have the server response headers, so we continue
+//              with cache write handling.
+//
+// Possible Next States From Here:
+// - handle_cache_operation_on_forward_server_response (if cache write succeeded)
+// - handle_no_cache_operation_on_forward_server_response (if cache write failed)
+//
+///////////////////////////////////////////////////////////////////////////////
+void
+HttpTransact::handle_deferred_cache_write_complete(State *s)

Review Comment:
   This one is hard to review :) Are there any existing tests that hits this code (with the setting enabled, obviously)? If not, this feels like we ought to have an autest (ideally start a new proxy-verify "framework" test?).



##########
include/proxy/http/HttpConfig.h:
##########
@@ -623,6 +623,11 @@ struct OverridableHttpConfigParams {
 
   MgmtByte cache_open_write_fail_action = 0;
 
+  ////////////////////////////////////////////////
+  // Defer cache open write until response hdrs //

Review Comment:
   Probably a useful comment, but this is almost too short :) Shouldn't it be something like "...until response headers are received" ?



##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3378,6 +3434,13 @@ HttpTransact::HandleCacheOpenReadMiss(State *s)
     s->cache_info.action = CacheAction_t::NO_ACTION;
   } else if (s->api_server_response_no_store) { // plugin may have decided not to cache the response
     s->cache_info.action = CacheAction_t::NO_ACTION;
+  } else if (s->txn_conf->cache_defer_write_on_miss) {
+    // Defer cache open write until after response headers are received.
+    // This avoids cache overhead for non-cacheable responses but may

Review Comment:
   In general,  I find Claud's comments to be overly verbose. If needed, this feels like it can be a one-line. It's pretty clear that 
   
   ```
     } else if (s->txn_conf->cache_defer_write_on_miss) {
   ```
   
   will "defer cache open write" ... If the code is self documenting, we shouldn't state the obvious. I'd suggest you look through all the comments (or ask to) and remove the comments that are obvious.



##########
src/records/RecordsConfig.cc:
##########
@@ -623,6 +623,13 @@ static constexpr RecordElement RecordsConfig[] =
   //       #  6 - retry cache read on write lock failure, if retries exhausted serve stale if allowed, otherwise goto origin
   {RECT_CONFIG, "proxy.config.http.cache.open_write_fail_action", RECD_INT, "0", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
   ,
+  //       #  defer_write_on_miss:

Review Comment:
   I don't know that we need this verbose documentation here either, either leave it out, or refer to the Docs section as before.



##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3333,6 +3333,62 @@ HttpTransact::handle_cache_write_lock_go_to_origin_continue(State *s)
   }
 }
 
+///////////////////////////////////////////////////////////////////////////////
+// Name       : handle_deferred_cache_write_complete
+// Description: Called after cache write opens for deferred cache write.
+//              We already have the server response headers, so we continue
+//              with cache write handling.
+//
+// Possible Next States From Here:
+// - handle_cache_operation_on_forward_server_response (if cache write succeeded)
+// - handle_no_cache_operation_on_forward_server_response (if cache write failed)
+//
+///////////////////////////////////////////////////////////////////////////////
+void
+HttpTransact::handle_deferred_cache_write_complete(State *s)
+{
+  TxnDbg(dbg_ctl_http_trans, "handle_deferred_cache_write_complete");
+  TxnDbg(dbg_ctl_http_seq, "Entering handle_deferred_cache_write_complete");
+
+  ink_assert(s->cache_info.action == CacheAction_t::PREPARE_TO_WRITE);
+
+  switch (s->cache_info.write_lock_state) {
+  case CacheWriteLock_t::SUCCESS:
+    // Cache write lock acquired successfully
+    TxnDbg(dbg_ctl_http_trans, "[hdcwc] cache write lock acquired");
+    SET_UNPREPARE_CACHE_ACTION(s->cache_info);
+    // Now we have WRITE action, handle the response with caching
+    handle_cache_operation_on_forward_server_response(s);
+    return;
+
+  case CacheWriteLock_t::FAIL:

Review Comment:
   From Claude, sounds like there's a bit of overlap between these two settings, that should at least be documented.
   
   - **[`HttpTransact.cc:3364`](https://github.com/apache/trafficserver/blob/21d2cd14f1f4b1dec5e04b3010cd96b544e79988/src/proxy/http/HttpTransact.cc#L3364)**: `handle_deferred_cache_write_complete` FAIL case does not consult `cache_open_write_fail_action`. The normal path in `handle_cache_write_lock` (line 3156) checks this setting and can return 502 via `ERROR_ON_MISS`, `ERROR_ON_MISS_STALE_ON_REVALIDATE`, or `ERROR_ON_MISS_OR_REVALIDATE`. The deferred path unconditionally treats FAIL as `LOCK_MISS` and serves the origin response. This is arguably correct (we already fetched from origin, so returning 502 would be wasteful), but the interaction between `defer_write_on_miss` and `cache_open_write_fail_action` needs to be documented as a known trade-off. Users relying on `ERROR_ON_MISS` for thundering-herd protection should understand that the deferred path bypasses it.
   



##########
src/proxy/http/HttpTransact.cc:
##########
@@ -4849,6 +4912,24 @@ HttpTransact::handle_no_cache_operation_on_forward_server_response(State *s)
   TxnDbg(dbg_ctl_http_trans, "(hncoofsr)");
   TxnDbg(dbg_ctl_http_seq, "Entering handle_no_cache_operation_on_forward_server_response");
 
+  // Check if we deferred the cache open write and the response is cacheable.
+  // If so, we need to open the cache for write now.
+  if (s->cache_open_write_deferred && s->txn_conf->cache_http) {
+    bool cacheable = is_response_cacheable(s, &s->hdr_info.client_request, &s->hdr_info.server_response);
+    TxnDbg(dbg_ctl_http_trans, "[hncoofsr] deferred cache write, response %s cacheable", cacheable ? "is" : "is not");
+
+    if (cacheable) {
+      // Response is cacheable - open the cache for write now

Review Comment:
   Again, stating the obvious in this comment, if (cacheable) is self explanatory.



##########
include/proxy/http/HttpTransact.h:
##########
@@ -697,6 +697,10 @@ class HttpTransact
     bool is_method_stats_incremented  = false;
     bool skip_ip_allow_yaml           = false;
 
+    /// True if we deferred opening the cache for write until after response headers.
+    /// This is an optimization to avoid cache overhead for non-cacheable responses.

Review Comment:
   The second comment here sounds like something that would be documented as part of the new setting. We know why we added this ...



##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3333,6 +3333,62 @@ HttpTransact::handle_cache_write_lock_go_to_origin_continue(State *s)
   }
 }
 
+///////////////////////////////////////////////////////////////////////////////

Review Comment:
   This is a good comment :) 



##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3333,6 +3333,62 @@ HttpTransact::handle_cache_write_lock_go_to_origin_continue(State *s)
   }
 }
 
+///////////////////////////////////////////////////////////////////////////////
+// Name       : handle_deferred_cache_write_complete
+// Description: Called after cache write opens for deferred cache write.
+//              We already have the server response headers, so we continue
+//              with cache write handling.
+//
+// Possible Next States From Here:
+// - handle_cache_operation_on_forward_server_response (if cache write succeeded)
+// - handle_no_cache_operation_on_forward_server_response (if cache write failed)
+//
+///////////////////////////////////////////////////////////////////////////////
+void
+HttpTransact::handle_deferred_cache_write_complete(State *s)
+{
+  TxnDbg(dbg_ctl_http_trans, "handle_deferred_cache_write_complete");
+  TxnDbg(dbg_ctl_http_seq, "Entering handle_deferred_cache_write_complete");
+
+  ink_assert(s->cache_info.action == CacheAction_t::PREPARE_TO_WRITE);
+
+  switch (s->cache_info.write_lock_state) {
+  case CacheWriteLock_t::SUCCESS:
+    // Cache write lock acquired successfully
+    TxnDbg(dbg_ctl_http_trans, "[hdcwc] cache write lock acquired");
+    SET_UNPREPARE_CACHE_ACTION(s->cache_info);
+    // Now we have WRITE action, handle the response with caching
+    handle_cache_operation_on_forward_server_response(s);
+    return;
+
+  case CacheWriteLock_t::FAIL:
+    // Could not get cache write lock, continue without caching
+    TxnDbg(dbg_ctl_http_trans, "[hdcwc] cache write lock failed, continuing without cache");
+    Metrics::Counter::increment(http_rsb.cache_open_write_fail_count);
+    s->cache_info.action       = CacheAction_t::NO_ACTION;
+    s->cache_info.write_status = CacheWriteStatus_t::LOCK_MISS;
+    break;
+
+  case CacheWriteLock_t::READ_RETRY:
+    // This shouldn't happen for deferred write since we don't have an object to read
+    TxnDbg(dbg_ctl_http_trans, "[hdcwc] unexpected READ_RETRY for deferred write");
+    s->cache_info.action       = CacheAction_t::NO_ACTION;
+    s->cache_info.write_status = CacheWriteStatus_t::LOCK_MISS;
+    break;
+
+  case CacheWriteLock_t::INIT:
+  default:
+    ink_release_assert(0);
+    break;
+  }
+
+  // If we get here, cache write failed - continue without caching
+  // The original handle_no_cache_operation_on_forward_server_response will be called

Review Comment:
   Well, yeah, I can see that handle_no_cache_operation_on_forward_server_response is called ... 



-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#issuecomment-3715428696

   [approve ci autest]


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#issuecomment-4070890317

   [approve ci autest]


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #12782: Optimize cache miss path with deferred cache write
URL: https://github.com/apache/trafficserver/pull/12782


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#issuecomment-3894502533

   retest this please


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#issuecomment-3948171356

   Seems this has conflicts now.


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Optimize cache miss path with deferred cache write [trafficserver]

Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #12782:
URL: https://github.com/apache/trafficserver/pull/12782#issuecomment-3715429793

   [approve ci autest]


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org