Fix memory tracking

[dfwilbanks395]

We track the size of materialized dataflow state to apply heuristics for evictions.

Some small issues regarding memory tracking

• base_row_size_from_comparison() does not track Range comparisons
• base_row_size takes the size of a Row::default() instead of Rows::default()
• We don’t track the overhead of the duplicate count in HashBag, though this is pretty insignificant.
• We incorrectly double count duplicate rows in MemoryState::insert() but do not double count when removing
• We don’t count the small overhead of Rc for duplicate rows across multiple SingleStates, though this is also pretty insignificant
• We return bytes_freed rather than bytes_freed + key_bytes from MemoryState::evict_keys()
• We ignore the overhead of KeyedState and PartialMap, though we might not want to try to account for this
• Inconsistent use of self.state_size in Domain::handle_exiction(). Among other issues, evicting from a reader does not always cause self.state_size to be updated.

[dfwilbanks395]

see https://gerrit.readyset.name/c/readyset/+/1054 for background here

[dfwilbanks395]

Do we track size only to make eviction decisions? Is there any another purpose of tracking materialized df state size? I am asking b/c there are alternative approaches which might work better especially when other components memory usage is taken into account (RocksDB): getting process RSS or using jemalloc stats (live allocations size)?

[glittershark]

• Inconsistent use of Domain::state_size. I checked the code and found that this value is never read, there are store/decrements, no reads. What is the purpose of updating this value during eviction it it is never read?

it's read here:

https://github.com/readysettech/readyset/blob/309bf5530043ca5062dd514833eca5b9b20643b8/readyset-server/src/worker/mod.rs#L534-L547

• On KeyedState and PartialMap overheads. They are not only data structures which adds up to MemoryState memory usage, there are others: IndexMap, for example. Calculating these overheads for every insert/remove operation is not feasible due to inner complexity of these data types- only as total overhead for MemoryState,which can be done in MemoryState::deep_size_of(). It's doable (not easy), but can result in some performance penalty (hard to estimate yet).

• HashBagvalue counter overhead. It's simple for a given HashBag, but to get total HashBagoverheads for MemoryState we need to iterate through all values (Rows) kept in this MemoryStateNot sure how fast or slow it can be. Again, we can not do increments/decrements on this overhead type during insert or remove.

Maybe instead of trying to get perfect memory accounting everywhere, we could look at what this overhead is (especially since I think it's either constant or bounded) and make the decision based on that. I don't think we need to be perfect, but I'm willing to be convinced otherwise.

Incorrectly double count duplicate rows in MemoryState::insert(). I did not find this in the code, but I found other small issue, which I fixed.

what's the question here?

[dfwilbanks395]

I think that if we call MemoryState::insert() twice with the same value of r each time, we will drop the second r instead of storing it since Rows is implemented with a HashBag

[dfwilbanks395]

Why do we call it twice?

We call it on every insert, so we could make two identical calls if the user has inserted a duplicate value.

…so if you insert row twice you will get V = 2

Yup, I wasn't clear, but this is where I think we end up double counting duplicates. We account for the memory of the Row on every insert. However, if we're inserting a duplicate row to the HashBag, we drop the Row and just increment the counter.

[glittershark]

The information is already propagated - the mem_size field is updated on MemoryState and read via the deep_size_of function in Domain

[VladRodionov]

Reopening this issue to investigate why system benchmarks time outs with memory limit set.