Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i#3068: API for accessing cache model metrics #5020

Merged
merged 10 commits into from
Aug 5, 2021
42 changes: 42 additions & 0 deletions clients/drcachesim/simulator/cache_simulator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,48 @@ cache_simulator_t::print_results()
return true;
}

int_least64_t
cache_simulator_t::get_cache_metric(unsigned level, metric_name_t metric, unsigned core,
cache_split_t split) const
{
caching_device_t *curr_cache;

if (core >= knobs_.num_cores) {
ERRMSG("Wrong core number: %u.\n", core);
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Xref the top-level thread: I would vote for returning an error. Could go so far as error enum types; or just return -1 (document that's the error return code) either w/ or w/o the print (could see the print useful if there's no errno; could see it annoying linked into a large system that maybe doesn't know the cache layout and is passing all possible levels and using return code to find the bounds of the layout -- there an errno would be nice).

Ditto below.

}

if (split == DATA) {
curr_cache = l1_dcaches_[core];
} else {
curr_cache = l1_icaches_[core];
}

for (size_t i = 1; i < level; i++) {
caching_device_t *parent = curr_cache->get_parent();

if (parent == NULL) {
ERRMSG("Wrong cache level: %u.\n", core);
return 0;
}
curr_cache = parent;
}
caching_device_stats_t *stats = curr_cache->get_stats();

if (stats == NULL) {
ERRMSG("Cache doesn't support statistics counting.");
return 0;
}

return stats->get_metric(metric);
}

const cache_simulator_knobs_t &
cache_simulator_t::get_knobs() const
{
return knobs_;
}

cache_t *
cache_simulator_t::create_cache(const std::string &policy)
{
Expand Down
9 changes: 9 additions & 0 deletions clients/drcachesim/simulator/cache_simulator.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
#include "cache.h"
#include "snoop_filter.h"

enum cache_split_t { DATA, INSTRUCTION };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Safer to put inside the class and not in the global namespace (xref #4343). Or see other suggestions for the enum below.


class cache_simulator_t : public simulator_t {
public:
// This constructor is used when the cache hierarchy is configured
Expand All @@ -60,12 +62,19 @@ class cache_simulator_t : public simulator_t {
bool
print_results() override;

int_least64_t
get_cache_metric(unsigned level, metric_name_t metric, unsigned core = 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would re-order the parameters with the metric first and grouping the 3 identifiers of which cache (level, core, split) together.

cache_split_t split = DATA) const;

// Exposed to make it easy to test
bool
check_warmed_up();
uint64_t
remaining_sim_refs() const;

const cache_simulator_knobs_t &
get_knobs() const;

protected:
// Create a cache_t object with a specific replacement policy.
virtual cache_t *
Expand Down
3 changes: 3 additions & 0 deletions clients/drcachesim/simulator/cache_stats.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ cache_stats_t::cache_stats_t(const std::string &miss_file, bool warmup_enabled,
, num_prefetch_hits_(0)
, num_prefetch_misses_(0)
{
stats_map_.emplace(FLUSHES, num_flushes_);
stats_map_.emplace(PREFETCH_HITS, num_prefetch_hits_);
stats_map_.emplace(PREFETCH_MISSES, num_prefetch_misses_);
}

void
Expand Down
9 changes: 9 additions & 0 deletions clients/drcachesim/simulator/caching_device_stats.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,15 @@ caching_device_stats_t::caching_device_stats_t(const std::string &miss_file,
} else
dump_misses_ = true;
}

stats_map_.emplace(HITS, num_hits_);
stats_map_.emplace(MISSES, num_misses_);
stats_map_.emplace(HITS_AT_RESET, num_hits_at_reset_);
stats_map_.emplace(MISSES_AT_RESET, num_misses_at_reset_);
stats_map_.emplace(CHILD_HITS_AT_RESET, num_child_hits_at_reset_);
stats_map_.emplace(CHILD_HITS, num_child_hits_);
stats_map_.emplace(INCLUSIVE_INVALIDATES, num_inclusive_invalidates_);
stats_map_.emplace(COHERENCE_INVALIDATES, num_coherence_invalidates_);
}

caching_device_stats_t::~caching_device_stats_t()
Expand Down
30 changes: 30 additions & 0 deletions clients/drcachesim/simulator/caching_device_stats.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@

#include "caching_device_block.h"
#include <string>
#include <map>
#include <stdint.h>
#ifdef HAS_ZLIB
# include <zlib.h>
Expand All @@ -49,6 +50,20 @@ enum invalidation_type_t {
INVALIDATION_COHERENCE,
};

enum metric_name_t {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the existing code has global scope enums but we're trying to avoid that going forward (xref #4343), and here we have very simple names that could conflict. We do require C++11 so this could be a scoped enum (typedef enum class metric_name_t) where it would require the prefix metric_name_t::HITS; or at least use a prefix like the existing enums so METRIC_HITS; or something, to avoid global symbols with very simple names like HITS which could conflict (this code is statically linked into some very large code bases).

HITS,
MISSES,
HITS_AT_RESET,
MISSES_AT_RESET,
CHILD_HITS,
CHILD_HITS_AT_RESET,
INCLUSIVE_INVALIDATES,
COHERENCE_INVALIDATES,
PREFETCH_HITS,
PREFETCH_MISSES,
FLUSHES
};

class caching_device_stats_t {
public:
explicit caching_device_stats_t(const std::string &miss_file,
Expand Down Expand Up @@ -81,6 +96,17 @@ class caching_device_stats_t {
virtual void
invalidate(invalidation_type_t invalidation_type);

int_least64_t
get_metric(metric_name_t metric) const
{
if (stats_map_.find(metric) != stats_map_.end()) {
return stats_map_.at(metric);
} else {
ERRMSG("Wrong metric name.\n");
return 0;
}
}

protected:
bool success_;

Expand Down Expand Up @@ -115,6 +141,10 @@ class caching_device_stats_t {
// Print out write invalidations if cache is coherent.
bool is_coherent_;

// References to the properties with statistics are held in the map with the
// statistic name as the key. Sample map element: {HITS, num_hits_}
std::map<metric_name_t, int_least64_t &> stats_map_;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some kind of test. Maybe in drcachesim_unit_tests or a new similar test? Xref #4842.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added unit test in the drcachesim_unit_tests. I couldn't test Invalidates metric properly because of the bug/issue that I described here: #5031

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, best to put a XXX i#5031: or TODO i#5031: comment in the test about adding that metric.

// We provide a feature of dumping misses to a file.
bool dump_misses_;
#ifdef HAS_ZLIB
Expand Down
63 changes: 63 additions & 0 deletions clients/drcachesim/tests/drcachesim_unit_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
// Unit tests for drcachesim
#include <iostream>
#include <cstdlib>
#include <assert.h>
#include "simulator/cache_simulator.h"
#include "../common/memref.h"

Expand Down Expand Up @@ -135,9 +136,71 @@ unit_test_sim_refs()
}
}

void
unit_test_metrics_API()
{
cache_simulator_knobs_t knobs = make_test_knobs();
cache_simulator_t cache_sim(knobs);

memref_t ref;
ref.data.type = TRACE_TYPE_WRITE;
ref.data.addr = 0;
ref.data.size = 8;

for (int i = 0; i < 4; i++) {
if (!cache_sim.process_memref(ref)) {
std::cerr << "drcachesim unit_test_metrics_API failed: "
<< cache_sim.get_error_string() << "\n";
exit(1);
}
}
assert(cache_sim.get_cache_metric(1, MISSES, 0, DATA) == 1);
assert(cache_sim.get_cache_metric(1, HITS, 0, DATA) == 3);

ref.data.type = TRACE_TYPE_INSTR;

for (int i = 0; i < 4; i++) {
if (!cache_sim.process_memref(ref)) {
std::cerr << "drcachesim unit_test_metrics_API failed: "
<< cache_sim.get_error_string() << "\n";
exit(1);
}
}
assert(cache_sim.get_cache_metric(1, MISSES, 0, INSTRUCTION) == 1);
assert(cache_sim.get_cache_metric(1, HITS, 0, INSTRUCTION) == 3);

assert(cache_sim.get_cache_metric(2, MISSES) == 1);
assert(cache_sim.get_cache_metric(2, HITS) == 1);

ref.data.type = TRACE_TYPE_PREFETCH;
ref.data.addr += 64;

for (int i = 0; i < 4; i++) {
if (!cache_sim.process_memref(ref)) {
std::cerr << "drcachesim unit_test_metrics_API failed: "
<< cache_sim.get_error_string() << "\n";
exit(1);
}
}
assert(cache_sim.get_cache_metric(1, PREFETCH_MISSES, 0, DATA) == 1);
assert(cache_sim.get_cache_metric(1, PREFETCH_HITS, 0, DATA) == 3);

ref.data.type = TRACE_TYPE_DATA_FLUSH;

for (int i = 0; i < 4; i++) {
if (!cache_sim.process_memref(ref)) {
std::cerr << "drcachesim unit_test_metrics_API failed: "
<< cache_sim.get_error_string() << "\n";
exit(1);
}
}
assert(cache_sim.get_cache_metric(2, FLUSHES) == 4);
}

int
main(int argc, const char *argv[])
{
unit_test_metrics_API();
unit_test_warmup_fraction();
unit_test_warmup_refs();
unit_test_sim_refs();
Expand Down