i#8026 histogram cleanup: Remove shifts from histogram map (#8033)

Removes the pointless shifts of addresses into cache lines in the
drmemtrace histogram tool, as it's using a map in any case.

Also stops printing if there are fewer results than the requested count.
Adds a test of this.

Finally, makes cmp() and back_align() class functions so they can be
used in subclasses.

Fixes #8026

Author: derekbruening

clients/drcachesim/tests/histogram_test.cpp

@@ -38,6 +38,8 @@
 
 #include <iostream>
 #include <optional>
+#include <regex>
+#include <sstream>
 #include <vector>
 
 #include "../tools/histogram.h"
@@ -84,9 +86,7 @@ bool
 check_parallel_reduce()
 {
     static constexpr unsigned int LINE_SIZE = 64;
-    // XXX i#8026: Remove this shift to simplify storage.
-    static constexpr unsigned int LINE_SHIFT = 6;
-    histogram_t tool(LINE_SIZE, /*report_top=*/5, /*verbose=*/0);
+    histogram_t tool(LINE_SIZE, /*report_top=*/10, /*verbose=*/0);
     std::vector<memref_t> memrefs = {
         gen_instr(1, 20 * LINE_SIZE),
         gen_data(1, /*load=*/true, 10 * LINE_SIZE, 8),
@@ -120,23 +120,52 @@ check_parallel_reduce()
             std::cerr << "failed to obtain icache counts\n";
             return false;
         }
-        // XXX i#8026: Remove this shift to simplify storage.
-        if ((*icache_res)[(20 * LINE_SIZE) >> LINE_SHIFT] != 2 ||
-            (*icache_res)[(21 * LINE_SIZE) >> LINE_SHIFT] != 1) {
-            std::cerr << "incorrect icache counts: got "
-                      << (*icache_res)[(20 * LINE_SIZE) >> LINE_SHIFT] << ","
-                      << (*icache_res)[(21 * LINE_SIZE) >> LINE_SHIFT]
-                      << " instead of 2,1\n";
+        if ((*icache_res)[20 * LINE_SIZE] != 2 || (*icache_res)[21 * LINE_SIZE] != 1) {
+            std::cerr << "incorrect icache counts: got " << (*icache_res)[20 * LINE_SIZE]
+                      << "," << (*icache_res)[21 * LINE_SIZE] << " instead of 2,1\n";
             return false;
         }
         return true;
     };
     if (!check_icache())
         return false;
+
     // Do another reduce inside print_results, testing that multiple reduces
     // do not change the result. The get_icache_counts() will also do another
     // reduce.
+    // While at it, we test that printing stops before hitting the 10 top
+    // results we requested when there aren't 10 total.
+    std::stringstream capture;
+    std::streambuf *prior = std::cerr.rdbuf(capture.rdbuf());
     tool.print_results();
+    std::string res = capture.str();
+    std::cerr.rdbuf(prior);
+    // The order of same-count items can vary so we use a regex.
+    const char *expect = R"DELIM(Cache line histogram tool results:
+icache: 5 unique cache lines
+dcache: 8 unique cache lines
+icache top 10
+             0x500: 2
+             0x..0: 1
+             0x..0: 1
+             0x..0: 1
+             0x..0: 1
+dcache top 10
+             0x280: 2
+             0x..0: 1
+             0x..0: 1
+             0x..0: 1
+             0x..0: 1
+             0x..0: 1
+             0x..0: 1
+             0x..0: 1
+)DELIM";
+    if (!std::regex_search(res, std::regex(expect))) {
+        std::cerr << "print_results output |" << res << "| did not match expected |"
+                  << expect << "\n";
+        return false;
+    }
+
     if (!check_icache())
         return false;
     return true;

clients/drcachesim/tools/histogram.cpp

@@ -66,7 +66,6 @@ histogram_t::histogram_t(unsigned int line_size, unsigned int report_top,
     : knob_line_size_(line_size)
     , knob_report_top_(report_top)
 {
-    line_size_bits_ = compute_log2((int)line_size);
 }
 
 histogram_t::~histogram_t()
@@ -110,12 +109,6 @@ histogram_t::parallel_shard_exit(void *shard_data)
     return true;
 }
 
-static inline addr_t
-back_align(addr_t addr, addr_t align)
-{
-    return addr & ~(align - 1);
-}
-
 bool
 histogram_t::parallel_shard_memref(void *shard_data, const memref_t &memref)
 {
@@ -141,8 +134,7 @@ histogram_t::parallel_shard_memref(void *shard_data, const memref_t &memref)
     for (addr_t addr = back_align(start_addr, knob_line_size_);
          addr < start_addr + size && addr < addr + knob_line_size_ /* overflow */;
          addr += knob_line_size_) {
-        // XXX i#8026: Remove this shift to simplify storage.
-        ++(*cache_map)[addr >> line_size_bits_];
+        ++(*cache_map)[addr];
     }
     return true;
 }
@@ -165,7 +157,8 @@ histogram_t::process_memref(const memref_t &memref)
 }
 
 bool
-cmp(const std::pair<addr_t, uint64_t> &l, const std::pair<addr_t, uint64_t> &r)
+histogram_t::cmp(const std::pair<addr_t, uint64_t> &l,
+                 const std::pair<addr_t, uint64_t> &r)
 {
     return l.second > r.second;
 }
@@ -228,20 +221,28 @@ histogram_t::print_results()
     std::partial_sort_copy(reduced_.icache_map.begin(), reduced_.icache_map.end(),
                            top.begin(), top.end(), cmp);
     std::cerr << "icache top " << top.size() << "\n";
-    for (std::vector<std::pair<addr_t, uint64_t>>::iterator it = top.begin();
-         it != top.end(); ++it) {
-        std::cerr << std::setw(18) << std::hex << std::showbase << (it->first << 6)
-                  << ": " << std::dec << it->second << "\n";
+    for (const auto &[addr, count] : top) {
+        if (count == 0) {
+            // If total observed elements are fewer than knob_report_top_, avoid
+            // printing lines of 0's.
+            break;
+        }
+        std::cerr << std::setw(18) << std::hex << std::showbase << addr << ": "
+                  << std::dec << count << "\n";
     }
     top.clear();
     top.resize(knob_report_top_);
     std::partial_sort_copy(reduced_.dcache_map.begin(), reduced_.dcache_map.end(),
                            top.begin(), top.end(), cmp);
     std::cerr << "dcache top " << top.size() << "\n";
-    for (std::vector<std::pair<addr_t, uint64_t>>::iterator it = top.begin();
-         it != top.end(); ++it) {
-        std::cerr << std::setw(18) << std::hex << std::showbase << (it->first << 6)
-                  << ": " << std::dec << it->second << "\n";
+    for (const auto &[addr, count] : top) {
+        if (count == 0) {
+            // If total observed elements are fewer than knob_report_top_, avoid
+            // printing lines of 0's.
+            break;
+        }
+        std::cerr << std::setw(18) << std::hex << std::showbase << addr << ": "
+                  << std::dec << count << "\n";
     }
     // Reset the i/o format for subsequent tool invocations.
     std::cerr << std::dec;

clients/drcachesim/tools/histogram.h

@@ -92,9 +92,17 @@ class histogram_t : public analysis_tool_t {
         std::string error;
     };
 
+    static bool
+    cmp(const std::pair<addr_t, uint64_t> &l, const std::pair<addr_t, uint64_t> &r);
+
+    static inline addr_t
+    back_align(addr_t addr, addr_t align)
+    {
+        return addr & ~(align - 1);
+    }
+
     unsigned int knob_line_size_;
     unsigned int knob_report_top_; /* most accessed lines */
-    size_t line_size_bits_;
     static const std::string TOOL_NAME;
     std::unordered_map<int, shard_data_t *> shard_map_;
     // This mutex is only needed in parallel_shard_init.  In all other accesses to