Feature/memory tracking #80
Conversation
nathanhhughes
left a comment
nathanhhughes
left a comment
There was a problem hiding this comment.
LGTM! Some suggestions on the unit tests to make them a little bit more change-proof, but not super important
tests/utest_memory_usage.cpp
Outdated
| EXPECT_LT(ratio, 2.0); | ||
| EXPECT_GT(ratio, 1.9); |
There was a problem hiding this comment.
(minor) I'd maybe make this just EXPECT_GT(ratio, 1.0) to avoid having to update the unit test if we update how we compute memory (e.g., if we exactly compute the node attributes memory requirements)
| expected_size += 458; | ||
| EXPECT_EQ(graph.memoryUsage(), expected_size); | ||
|
|
||
| // Add a partitioned layer. | ||
| graph.addLayer(2, 1, "test_layer_partition"); | ||
| expected_size += 120; |
There was a problem hiding this comment.
Why does adding a partition add more memory than just the layer? I'd maybe expect a couple more bytes for the longer name, but not ~300 bytes
There was a problem hiding this comment.
The non-partition layer actually adds more, I assume it's a matter of the ordering (maybe the first layer initializes data-tracking structures etc?). Otherwise I don't have a great idea why they should be different, I was also surprised that it is that much 😆
tests/utest_memory_usage.cpp
Outdated
| for (size_t i = 0; i < 100; ++i) { | ||
| edge_container.insert(i, i + 1, std::make_unique<EdgeAttributes>(i * 0.1)); | ||
| } | ||
| expected_size += 100 * 100; |
There was a problem hiding this comment.
It would be good to resuse memoryUsage for the EdgeAttributes to avoid the tests failing if the edge attribute struct changes (which should work correctly because you tested that previously)
|
|
||
| TEST(MemoryUsage, NodeAttributes) { | ||
| NodeAttributes attrs; | ||
| size_t expected_size = 47; |
There was a problem hiding this comment.
idk how to make this robust to the attributes changing, so this is probably fine, but worth thinking about in the future
tests/utest_memory_usage.cpp
Outdated
| meta_contents["type"] = "Test node"; | ||
| meta_contents["value"] = 123; | ||
| attrs.metadata.set(meta_contents); | ||
| expected_size = 109; |
There was a problem hiding this comment.
Ideally we do 47 + meta_contents.memoryUsage() so this is easier to update if the attribute definition changes
* first implementation of memory tracking * minor cleanup * clean up utest with review feedback and fix meta data double counting
* first implementation of memory tracking * minor cleanup * clean up utest with review feedback and fix meta data double counting
* first implementation of memory tracking * minor cleanup * clean up utest with review feedback and fix meta data double counting
* first implementation of memory tracking * minor cleanup * clean up utest with review feedback and fix meta data double counting
@marcusabate This should expose all of the data / interfaces you need if you want to create more detailed memory analyses. You can look through the individual implementations if you want to see how the different memory is counted.