Uh oh!
There was an error while loading. Please reload this page.
Add memory usage statistics for slabs and allocation classes - #91
Conversation
c880d56 to
891ad6bCompareHi @igchor, most of the metrics have the name and value separated by a single colon like: For the line below, I'm thinking how we would parse this output into 'name, value' for prometheus If we want to follow the same format as other metrics maybe we could use this format: Does it make sense to change the stats output to use this format or something like this? Our existing scripts will automatically collect new metrics output in this format. |
guptask
commented
Jul 6, 2022
should this name have approx term in it as well ? |
guptask
commented
Jul 6, 2022
Do you want to use memory_order_seq_cst order here for atomic store ? Otherwise store() is needed if you want to use something like memory_order_release order. |
guptask
commented
Jul 6, 2022
same as above comment about memory order if using assignment operator instead of atomic store(). |
guptask
commented
Jul 6, 2022
Cosmetic review : I suggest using curly braces even if there is one line after if statement. |
am not sure this line will pass the clang-format check. the character count is high. |
guptask
left a comment
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @byrnedj and @igchor)
igchor
left a comment
There was a problem hiding this comment.
@raema Ok, I changed it to how you suggested. Please let me now if this is OK.
Reviewable status: 7 of 15 files reviewed, 5 unresolved discussions (waiting on @byrnedj and @guptask)
cachelib/allocator/CacheAllocator-inl.h line 2510 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
should this name have approx term in it as well ?
Done.
cachelib/allocator/memory/AllocationClass.cpp line 54 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
Do you want to use memory_order_seq_cst order here for atomic store ? Otherwise store() is needed if you want to use something like memory_order_release order.
It's just for readability, the order doesn't matter here since it's ctor.
cachelib/allocator/memory/AllocationClass.cpp line 126 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
same as above comment about memory order if using assignment operator instead of atomic store().
Same as above.
cachelib/allocator/memory/AllocationClass.cpp line 741 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
Cosmetic review : I suggest using curly braces even if there is one line after if statement.
Done.
cachelib/allocator/tests/CacheBaseTest.cpp line 36 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
am not sure why this line passed the clang-format check ? the character count is high.
Done.
raema
commented
Jul 7, 2022
This output looks good. Thanks! |
| } | ||
| } | ||
| auto maxBatch = *std::max_element(batches.begin(), batches.end()); |
There was a problem hiding this comment.
will segfault if batches.size() == 0
byrnedj
left a comment
There was a problem hiding this comment.
line 52 of PromotionStrategey.h will segfault if vector is empty. did you mean to include PromotionStrategy.h on this PR?
igchor
left a comment
There was a problem hiding this comment.
Good catch, I commited this file by mistake.
Reviewable status: 7 of 15 files reviewed, 6 unresolved discussions (waiting on @byrnedj and @guptask)
cachelib/allocator/PromotionStrategy.h line 52 at r2 (raw file):
Previously, byrnedj (Daniel Byrne) wrote…
will segfault if batches.size() == 0
Done.
byrnedj
left a comment
There was a problem hiding this comment.
Reviewable status: 7 of 15 files reviewed, 5 unresolved discussions (waiting on @byrnedj and @guptask)
Track latency of per item eviction/promotion between memory tiers
Printing those stats can be enabled by passing
-report_memory_usage_statsto cachebenchSample output:
This change is