Skip to content

[RFC] Reimplement tachanka_ranking_touch_all function #33

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.

Sign up for GitHub

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

Open
PatKamin wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[RFC] Reimplement tachanka_ranking_touch_all function #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions include/memkind/internal/tachanka.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,14 @@ bool tachanka_ranking_event_pop(EventEntry_t *event);
void tachanka_ranking_touch_all(__u64 timestamp, double add_hotness);

/// \brief Getter for type's frequency
/// \param index index of a type in ttypes list
double tachanka_get_frequency(size_t index);
/// \param freq pointer to save frequencies
/// \param size number of different types
void *tachanka_get_frequency(double *freqs, size_t size);

/// \brief Getter for type's timestamp state
/// \param index index of a type in ttypes list
TimestampState_t tachanka_get_timestamp_state(size_t index);
/// \param timestamp pointer to save timestamps
/// \param size number of different types
void *tachanka_get_timestamp_state(TimestampState_t *timestamp, size_t size);

struct ttype {
uint64_t hash;
Expand Down
62 changes: 50 additions & 12 deletions src/tachanka.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,26 +579,64 @@ MEMKIND_EXPORT bool tachanka_ranking_event_pop(EventEntry_t *event)
return ranking_event_pop(&ranking_event_buff, event);
}

struct touch_data {
Copy link
Collaborator

@maciekpac maciekpac Dec 1, 2021

Choose a reason for hiding this comment

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

Why is this structure declared here, not at the top?

__u64 timestamp;
double add_hotness;
};

static int tachanka_ranking_touch_all_internal(uintptr_t key, void *value, void *privdata) {
uint64_t timestamp = ((struct touch_data *)privdata)->timestamp;
double add_hotness = ((struct touch_data *)privdata)->add_hotness;
ranking_touch(ranking, (struct ttype *)value, timestamp, add_hotness);
return 0;
}

MEMKIND_EXPORT void tachanka_ranking_touch_all(__u64 timestamp, double add_hotness)
{
// TODO re-implement this function
// add API for getting specific element from slab_allocator
// and re-implement it
// for (int i = 0; i < ntypes; ++i) {
// ranking_touch(ranking, &ttypes[i], timestamp, add_hotness);
// }
struct touch_data privdata = {timestamp, add_hotness};
critnib_iter(hash_to_type, 0, -1, *tachanka_ranking_touch_all_internal, &privdata);
}

struct frequencies {
Copy link
Collaborator

@maciekpac maciekpac Dec 1, 2021

Choose a reason for hiding this comment

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

Ditto

double *freq;
Copy link
Collaborator

@maciekpac maciekpac Dec 1, 2021

Choose a reason for hiding this comment

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

I think it would be clearer to use hotness instead of freq - frequency is there for historic reasons, currently hotness is not a direct measure of frequency.

size_t size;
};

static int tachanka_get_frequency_internal(uintptr_t key, void *value, void *freqs) {
static size_t i = 0;
*(((struct frequencies *)freqs)->freq + i) = ((struct ttype *)value)->f;
i++;
if (i == ((struct frequencies *)freqs)->size) {
Copy link
Collaborator

@maciekpac maciekpac Dec 1, 2021

Choose a reason for hiding this comment

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

If there are less ttypes than size, how do we know which entries are set and valid? Maybe we should store i as a field of struct frequencies - set it to zero before iterating over critnib and then iterate the way we do, without static variables?

Another cool feature would be to know if overflow occurred - maybe we can store two counters, one absolute (not reset to 0 on overflow) and the other one that actually handles indices - reset to zero on overflow?

This structure is not copied and only one instance is created per function call (stored on stack), additional counters have minimal overhead. In my mind, the code would also be more readable than with static int.

i = 0;
}
return 0;
}

// Getter used in the tachanka_check_ranking_touch_all test
MEMKIND_EXPORT double tachanka_get_frequency(size_t index) {
// TODO: re-implement (add API in slab_allocator first!)
// return ttypes[index].f;
MEMKIND_EXPORT void *tachanka_get_frequency(double *freqs, size_t size) {
struct frequencies frequencies = {freqs, size};
critnib_iter(hash_to_type, 0, -1, *tachanka_get_frequency_internal, &frequencies);
Copy link
Collaborator

@maciekpac maciekpac Dec 1, 2021

Choose a reason for hiding this comment

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

Please check previous comment - it applies to this place as well (extension of struct frequencies).

return 0;
}

struct timestamps {
TimestampState_t *timestamp;
size_t size;
};

static int tachanka_get_timestamp_state_internal(uintptr_t key, void *value, void *ts) {
static size_t i = 0;
Copy link
Collaborator

@maciekpac maciekpac Dec 1, 2021

Choose a reason for hiding this comment

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

The remark is exactly the same as the one for tachanka_get_frequency_internal

*(((struct timestamps *)ts)->timestamp + i) = ((struct ttype *)value)->timestamp_state;
i++;
if (i == ((struct timestamps *)ts)->size) {
i = 0;
}
return 0;
}

// Getter used in the tachanka_check_ranking_touch_all test
MEMKIND_EXPORT TimestampState_t tachanka_get_timestamp_state(size_t index) {
// TODO: re-implement (add API in slab_allocator first!)
// return ttypes[index].timestamp_state;
MEMKIND_EXPORT void *tachanka_get_timestamp_state(TimestampState_t *timestamps, size_t size) {
struct timestamps ts = {timestamps, size};
Copy link
Collaborator

@maciekpac maciekpac Dec 1, 2021

Choose a reason for hiding this comment

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

Ditto - possible update of struct timestamps

critnib_iter(hash_to_type, 0, -1, *tachanka_get_timestamp_state_internal, &ts);
return 0;
}
30 changes: 20 additions & 10 deletions test/memkind_memtier_hotness_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ INSTANTIATE_TEST_CASE_P(numObjsParam, MemkindMemtierHotnessTest,

TEST_F(MemkindMemtierHotnessTest, check_ranking_touch_all) {
const int MALLOC_COUNT = 5;
double freqs[MALLOC_COUNT];
__u64 hotness_measure_window = 1000000000; // equals to the hotness_measure_window defined in pebs_init()
__u64 wait_time = hotness_measure_window + 1;
std::vector<void *> malloc_vec;
Expand All @@ -288,45 +289,54 @@ TEST_F(MemkindMemtierHotnessTest, check_ranking_touch_all) {
malloc_vec.push_back(ptr);
}


sleep(1); // wait for pebs_monitor() to register new types from memtier_mallocs

#if HOTNESS_POLICY == HOTNESS_POLICY_TIME_WINDOW
TimestampState_t timestamp_states[MALLOC_COUNT];

tachanka_get_timestamp_state(timestamp_states, MALLOC_COUNT);
for (size_t i = 0; i < MALLOC_COUNT; ++i) {
ASSERT_EQ(tachanka_get_timestamp_state(i), TIMESTAMP_NOT_SET);
ASSERT_EQ(timestamp_states[i], TIMESTAMP_NOT_SET);
}
tachanka_ranking_touch_all(wait_time, 0); // set timestamp state to TIMESTAMP_INIT
tachanka_get_frequency(freqs, MALLOC_COUNT);
tachanka_get_timestamp_state(timestamp_states, MALLOC_COUNT);
for (size_t i = 0; i < MALLOC_COUNT; ++i) {
ASSERT_EQ(tachanka_get_frequency(i), 0);
ASSERT_EQ(freqs[i], 0);
}
for (size_t i = 0; i < MALLOC_COUNT; ++i) {
ASSERT_EQ(tachanka_get_timestamp_state(i), TIMESTAMP_INIT);
ASSERT_EQ(timestamp_states[i], TIMESTAMP_INIT);
}

tachanka_ranking_touch_all(2 * wait_time, 1e6); // set timestamp state to TIMESTAMP_INIT_DONE, set f to non-zero value
tachanka_get_frequency(freqs, MALLOC_COUNT);
tachanka_get_timestamp_state(timestamp_states, MALLOC_COUNT);
for (size_t i = 0; i < MALLOC_COUNT; ++i) {
ASSERT_EQ(tachanka_get_frequency(i), 0);
ASSERT_EQ(freqs[i], 0);
}
for (size_t i = 0; i < MALLOC_COUNT; ++i) {
ASSERT_EQ(tachanka_get_timestamp_state(i), TIMESTAMP_INIT_DONE);
ASSERT_EQ(timestamp_states[i], TIMESTAMP_INIT_DONE);
}

tachanka_ranking_touch_all(3 * wait_time, 0); // touch all ttypes without adding the hotness
tachanka_get_frequency(freqs, MALLOC_COUNT);
for (size_t i = 0; i < MALLOC_COUNT; ++i) {
ASSERT_GT(tachanka_get_frequency(i), 0);
ASSERT_GT(freqs[i], 0);
}
#elif HOTNESS_POLICY == HOTNESS_POLICY_EXPONENTIAL_COEFFS
double freqs[MALLOC_COUNT];
double updated_freqs[MALLOC_COUNT];

tachanka_ranking_touch_all(wait_time, 1e6); // set f to non-zero value
tachanka_get_frequency(freqs, MALLOC_COUNT);
for (size_t i = 0; i < MALLOC_COUNT; ++i) {
freqs[i] = tachanka_get_frequency(i);
ASSERT_GT(freqs[i], 0);
}

tachanka_ranking_touch_all(2 * wait_time, 0); // touch all ttypes without adding the hotness
tachanka_get_frequency(updated_freqs, MALLOC_COUNT);
for (size_t i = 0; i < MALLOC_COUNT; ++i) {
ASSERT_LT(tachanka_get_frequency(i), freqs[i]);
ASSERT_LT(updated_freqs[i], freqs[i]);
}
#endif

Expand Down