Copilot commented on code in PR #13061:
URL: https://github.com/apache/trafficserver/pull/13061#discussion_r3088123069
##########
src/iocore/cache/CacheVC.cc:
##########
@@ -479,6 +481,14 @@ CacheVC::handleRead(int /* event ATS_UNUSED */, Event * /*
e ATS_UNUSED */)
f.doc_from_ram_cache = true;
io.aio_result = io.aiocb.aio_nbytes;
+ // Try to promote this object to the RAM cache
+ Doc *doc = reinterpret_cast<Doc *>(buf->data());
+ if (_ram_cache_cutoff_check(doc)) {
+ bool http_copy_hdr = cache_config_ram_cache_compress && doc->hlen;
+ uint64_t o = dir_offset(&dir);
Review Comment:
In the last_open_read promotion path, `http_copy_hdr` is computed as
`cache_config_ram_cache_compress && doc->hlen`, which differs from the existing
`handleReadDone` logic (it also checks `doc->doc_type ==
CACHE_FRAG_TYPE_HTTP`). Since `Doc::hlen` exists for all doc types, this can
trigger the copy-in/copy-out behavior for non-HTTP docs (extra allocation /
different accounting) and diverges from the established promotion behavior.
Consider matching the `handleReadDone` condition (including the doc_type check)
so the promotion path is consistent.
```suggestion
bool http_copy_hdr =
cache_config_ram_cache_compress && doc->doc_type ==
CACHE_FRAG_TYPE_HTTP && doc->hlen;
uint64_t o = dir_offset(&dir);
```
##########
src/iocore/cache/CacheVC.cc:
##########
@@ -479,6 +481,14 @@ CacheVC::handleRead(int /* event ATS_UNUSED */, Event * /*
e ATS_UNUSED */)
f.doc_from_ram_cache = true;
io.aio_result = io.aiocb.aio_nbytes;
+ // Try to promote this object to the RAM cache
+ Doc *doc = reinterpret_cast<Doc *>(buf->data());
+ if (_ram_cache_cutoff_check(doc)) {
+ bool http_copy_hdr = cache_config_ram_cache_compress && doc->hlen;
+ uint64_t o = dir_offset(&dir);
+ stripe->ram_cache->put(read_key, buf.get(), doc->len, http_copy_hdr, o);
+ }
Review Comment:
The new RAM cache promotion on last_open_read hits changes cache residency
behavior and is performance-sensitive, but there doesn't appear to be any
regression/integration test coverage exercising the last_open_read path (and
verifying a subsequent `load_from_ram_cache()` hit / `ram_cache_hits`
increment). Adding a targeted cache regression test for this scenario would
help prevent future regressions in promotion behavior and cutoff handling.
--
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]
