zwoop commented on a change in pull request #7765:
URL: https://github.com/apache/trafficserver/pull/7765#discussion_r651193601
##########
File path: plugins/cache_promote/lru_policy.cc
##########
@@ -72,101 +112,130 @@ LRUPolicy::doPromote(TSHttpTxn txnp)
{
LRUHash hash;
LRUMap::iterator map_it;
- char *url = nullptr;
- int url_len = 0;
- bool ret = false;
- TSMBuffer request;
- TSMLoc req_hdr;
-
- if (TS_SUCCESS == TSHttpTxnClientReqGet(txnp, &request, &req_hdr)) {
- TSMLoc c_url = TS_NULL_MLOC;
-
- // Get the cache key URL (for now), since this has better lookup behavior
when using
- // e.g. the cachekey plugin.
- if (TS_SUCCESS == TSUrlCreate(request, &c_url)) {
- if (TS_SUCCESS == TSHttpTxnCacheLookupUrlGet(txnp, request, c_url)) {
- url = TSUrlStringGet(request, c_url, &url_len);
- TSHandleMLocRelease(request, TS_NULL_MLOC, c_url);
- }
- }
- TSHandleMLocRelease(request, TS_NULL_MLOC, req_hdr);
- }
+ bool ret = false;
- // Generally shouldn't happen ...
- if (!url) {
+ if (!hash.initFromUrl(txnp)) {
return false;
}
- TSDebug(PLUGIN_NAME, "LRUPolicy::doPromote(%.*s%s)", url_len > 100 ? 100 :
url_len, url, url_len > 100 ? "..." : "");
- hash.init(url, url_len);
- TSfree(url);
-
// We have to hold the lock across all list and hash access / updates
TSMutexLock(_lock);
map_it = _map.find(&hash);
if (_map.end() != map_it) {
+ auto &[map_key, map_val] = *map_it;
+ auto &[val_key, val_hits, val_bytes] = *(map_it->second);
+
+ // This is beacuse compilers before gcc 8 aren't smart enough to ignore
the unused structured bindings
+ (void)val_key;
+
// We have an entry in the LRU
TSAssert(_list_size > 0); // mismatch in the LRUs hash and list, shouldn't
happen
- incrementStat(lru_hit_id, 1);
- if (++(map_it->second->second) >= _hits) {
+ incrementStat(_lru_hit_id, 1);
+ if (++val_hits >= _hits || (_bytes > 0 && val_bytes > _bytes)) {
// Promoted! Cleanup the LRU, and signal success. Save the promoted
entry on the freelist.
TSDebug(PLUGIN_NAME, "saving the LRUEntry to the freelist");
- _freelist.splice(_freelist.begin(), _list, map_it->second);
+ _freelist.splice(_freelist.begin(), _list, map_val);
++_freelist_size;
--_list_size;
- _map.erase(map_it->first);
- incrementStat(promoted_id, 1);
- incrementStat(freelist_size_id, 1);
- decrementStat(lru_size_id, 1);
+ _map.erase(map_key);
+ incrementStat(_promoted_id, 1);
+ incrementStat(_freelist_size_id, 1);
+ decrementStat(_lru_size_id, 1);
ret = true;
} else {
// It's still not promoted, make sure it's moved to the front of the list
- TSDebug(PLUGIN_NAME, "still not promoted, got %d hits so far",
map_it->second->second);
- _list.splice(_list.begin(), _list, map_it->second);
+ TSDebug(PLUGIN_NAME, "still not promoted, got %d hits so far and %"
PRId64 " bytes", val_hits, val_bytes);
+ _list.splice(_list.begin(), _list, map_val);
}
} else {
// New LRU entry for the URL, try to repurpose the list entry as much as
possible
- incrementStat(lru_miss_id, 1);
+ incrementStat(_lru_miss_id, 1);
if (_list_size >= _buckets) {
TSDebug(PLUGIN_NAME, "repurposing last LRUHash entry");
_list.splice(_list.begin(), _list, --_list.end());
- _map.erase(&(_list.begin()->first));
- incrementStat(lru_vacated_id, 1);
+ _map.erase(&(std::get<0>(*_list.begin()))); // Get the hash from the
first list element
+ incrementStat(_lru_vacated_id, 1);
} else if (_freelist_size > 0) {
TSDebug(PLUGIN_NAME, "reusing LRUEntry from freelist");
_list.splice(_list.begin(), _freelist, _freelist.begin());
--_freelist_size;
++_list_size;
- incrementStat(lru_size_id, 1);
- decrementStat(freelist_size_id, 1);
+ incrementStat(_lru_size_id, 1);
+ decrementStat(_freelist_size_id, 1);
} else {
TSDebug(PLUGIN_NAME, "creating new LRUEntry");
_list.push_front(NULL_LRU_ENTRY);
++_list_size;
- incrementStat(lru_size_id, 1);
+ incrementStat(_lru_size_id, 1);
}
// Update the "new" LRUEntry and add it to the hash
- _list.begin()->first = hash;
- _list.begin()->second = 1;
- _map[&(_list.begin()->first)] = _list.begin();
+ *_list.begin() = {hash, 1, 0};
+ _map[&(std::get<0>(*_list.begin()))] = _list.begin();
}
TSMutexUnlock(_lock);
+ // If we didn't promote, and we want to count bytes, save away the
calculated hash for later use
+ if (false == ret && countBytes()) {
+ TSUserArgSet(txnp, TXN_ARG_IDX, static_cast<void *>(new LRUHash(hash)));
+ } else {
+ TSUserArgSet(txnp, TXN_ARG_IDX, nullptr);
+ }
+
return ret;
}
+void
+LRUPolicy::addBytes(TSHttpTxn txnp)
+{
+ LRUHash *hash = static_cast<LRUHash *>(TSUserArgGet(txnp, TXN_ARG_IDX));
+
+ if (hash) {
+ LRUMap::iterator map_it;
+
+ // We have to hold the lock across all list and hash access / updates
+ TSMutexLock(_lock);
+ map_it = _map.find(hash);
+ if (_map.end() != map_it) {
+ TSMBuffer resp;
+ TSMLoc resp_hdr;
+
+ if (TS_SUCCESS == TSHttpTxnServerRespGet(txnp, &resp, &resp_hdr)) {
+ TSMLoc field_loc = TSMimeHdrFieldFind(resp, resp_hdr,
TS_MIME_FIELD_CONTENT_LENGTH, TS_MIME_LEN_CONTENT_LENGTH);
+
+ if (field_loc) {
+ auto &[val_key, val_hits, val_bytes] = *(map_it->second);
+ int64_t cl =
TSMimeHdrFieldValueInt64Get(resp, resp_hdr, field_loc, -1);
+
+ // This is beacuse compilers before gcc 8 aren't smart enough to
ignore the unused structured bindings
+ (void)val_key, (void)val_hits;
+
+ val_bytes += cl;
+ TSDebug(PLUGIN_NAME, "Added %" PRId64 " bytes for LRU entry", cl);
+ TSHandleMLocRelease(resp, resp_hdr, field_loc);
+ }
+ TSHandleMLocRelease(resp, TS_NULL_MLOC, resp_hdr);
+ }
+ }
+ TSMutexUnlock(_lock);
+
+ // Delete the hash, and remove the pointer from the TXN user arg slot (to
be safe)
+ delete hash;
Review comment:
> What if the transaction fails before it gets to the hook where this is
called? I think you need to trigger on the TXN_CLOSE hook and delete this there.
Ok, will take a look, thanks!
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
