[GitHub] [trafficserver] ywkaras commented on a change in pull request #7765: Adds bytes counting as a trigger to the cache_promote LRU

[GitBox <gi...@apache.org>]

ywkaras commented on a change in pull request #7765:
URL: https://github.com/apache/trafficserver/pull/7765#discussion_r651264203



##########
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:
       Don't throw out the baby with the bathwater:  https://github.com/apache/trafficserver/blob/3d584af796bad1e9e3c03b2af5485b630ffedafb/plugins/xdebug/Cleanup.h#L21




-- 
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:
users@infra.apache.org