Uh oh!
There was an error while loading. Please reload this page.
perf: Replace BTreeMap with constant vector in MemoryManager - #240
Conversation
# Conflicts: # canbench_results.yml
Since we can have maximum 255 values, we can store those cheaply in a Vec and save time during the lookup.
|
I am not entirely sure about this change. It works well maybe now that we can only allocate new memories but what if we finally merge the change which allows to free a memory? Wouldn't that nullify the benefits of using a vector? EDIT: I'm talking about the open PR on freeing a memory where I'm expecting a |
frankdavid
commented
Nov 12, 2024
If you think we'd need to delete elements from the middle of the vector, that's not the case. We'll never delete elements from the outer We can remove a memory by simply taking its contents, eg: If this is confusing to read, I could wrap the whole thing into a struct which would have a similar API like stdlib |
dsarlis
commented
Nov 13, 2024
I see, yeah I was thinking we might need to remove in the middle of the vector but I get your point. In this case, I agree the change is beneficial. |
dragoljub-djuric
commented
Nov 13, 2024
@frankdavid nice change, thank you. I approve this change, but can we please wait for the memory freeing to first be merged, we have a couple of stacked PRs, and I hope that can be done next week. And after that, I will put a stamp here. Does that sound fine? |
frankdavid
commented
Nov 13, 2024
Sure, no problem. |
dragoljub-djuric
commented
Nov 18, 2024
Hey @frankdavid just an update, I believe the work on the memory manager will take more time than initially assumed, so please go forward and merge this PR if you think it is ready. |
Uh oh!
There was an error while loading. Please reload this page.
# Conflicts: # canbench_results.yml # src/btreemap/node.rs # src/btreemap/node/v1.rs # src/btreemap/node/v2.rs
frankdavid
commented
Nov 18, 2024
After merging recent changes from master, I no longer see significant performance improvements from this change, so I won't move forward with this PR for now. |
# Conflicts: # canbench_results.yml
After studying our code more, I realized that we didn't have any benchmarks that properly exercised the MemoryManager, that's why we didn't see any improvements. A few new benchmarks were added in #253 and now the effect of this PR is visible (see benchmarks with |
Since we can have maximum 255 memories, we can store those cheaply in a
Vecand save time during the lookup.