[RFC] Reimplement tachanka_ranking_touch_all function #33
Conversation
Reshape tests for this function and reimplement helper functions for tests purposes.
| @@ -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 { | |||
There was a problem hiding this comment.
Why is this structure declared here, not at the top?
| critnib_iter(hash_to_type, 0, -1, *tachanka_ranking_touch_all_internal, &privdata); | ||
| } | ||
|
|
||
| struct frequencies { |
| } | ||
|
|
||
| struct frequencies { | ||
| double *freq; |
There was a problem hiding this comment.
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.
| static size_t i = 0; | ||
| *(((struct frequencies *)freqs)->freq + i) = ((struct ttype *)value)->f; | ||
| i++; | ||
| if (i == ((struct frequencies *)freqs)->size) { |
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this comment.
Please check previous comment - it applies to this place as well (extension of struct frequencies).
| }; | ||
|
|
||
| static int tachanka_get_timestamp_state_internal(uintptr_t key, void *value, void *ts) { | ||
| static size_t i = 0; |
There was a problem hiding this comment.
The remark is exactly the same as the one for tachanka_get_frequency_internal
| // 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}; |
There was a problem hiding this comment.
Ditto - possible update of struct timestamps
Description
Types of changes
Checklist
Further comments
This change is