Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support memory freeing #95

Draft
wants to merge 86 commits into
base: main
Choose a base branch
from

Conversation

dragoljub-duric
Copy link
Contributor

@dragoljub-duric dragoljub-duric commented Jun 27, 2023

Problem:
Stable structures do not support memory freeing.

Solution:
To free memory, we’d need to add the following API to the memory manager:

fn free(memory: MemoryId) -> ();
Post Conditions:

  • The size of memory is zero.
  • The buckets used by memory are unallocated.
  • Currently, the memory manager allocates new buckets by growing its underlying memory. Instead, we should ensure that allocations of new buckets by the memory manager use all the unallocated buckets of memory first before growing the underlying memory.

To do:
Maybe in the future remove the external dependency BitVec and use bit operations instead.

@dragoljub-duric dragoljub-duric marked this pull request as ready for review July 12, 2023 12:01
@ielashi
Copy link
Contributor

ielashi commented Oct 17, 2023

@ielashi I saw that we have a new release. Can you please approve it now?

Hey @dragoljub-duric, I'd like to do some more testing given that we found a number of bugs previously in the PR, and given how important this code is, it's better to err on the side of caution.

Copy link
Contributor

@ielashi ielashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting back to this PR now :) I left some early comments, still taking a closer look.

src/memory_manager.rs Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
dragoljub-duric and others added 3 commits October 25, 2023 16:26
Co-authored-by: Islam El-Ashi <islam.elashi@dfinity.org>
@dragoljub-duric dragoljub-duric force-pushed the EXC-1404-memory-manager-support-freeing-a-memory branch from f8ee0ff to 2a75e50 Compare October 26, 2023 11:36
src/memory_manager.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
}
}

fn load_layout_v2(memory: M, header: Header) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been finding this function generally difficult to read. I'll add some pointers below on how to make it more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL.

src/memory_manager.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
src/memory_manager.rs Outdated Show resolved Hide resolved
@dragoljub-duric
Copy link
Contributor Author

@ielashi Can you please take another look?

@ielashi
Copy link
Contributor

ielashi commented Nov 2, 2023

@ielashi Can you please take another look?

@dragoljub-duric Thanks! From a code standpoint this looks good. The remaining concern I have is related to performance. Prior to this PR, memory performance operations were O(1), but with this change they would be O(# buckets used), correct? I don't yet know exactly how big of a deal this is, as we don't have benchmarks for that.

Let's discuss more in our 1:1 about next steps.

@ielashi
Copy link
Contributor

ielashi commented Dec 1, 2023

At a high-level, I think we should work on:

  1. Verifying the correctness of this change.
    For that, I think we need stronger test-cases for the MemoryManager. I suggest we (in a separate PR), add a fuzz test, in a way that's similar to this one, and we can run this for a couple of days with these changes to ensure things are working as expected.

  2. Understanding the performance impact of this change.

  • For that, I think we need to introduce (also in separate PR) the CI changes to track our benchmarks, just as we did with the bitcoin-canister repo.
  • We can then also add a benchmark where we allocate a large number of buckets and see how this current PR affects the performance of that benchmark.

@ielashi
Copy link
Contributor

ielashi commented Dec 20, 2023

We had a discussion offline and agreed that this approach may not be the best approach. This design was intended to keep the memory manager header bound to 64KiB to make migration from V1 simple, but that came with the additional complexity of using 15-bits for bucket indexes, and also a performance degradation where the grow memory operation can be up to 3 orders of magnitude more expensive.

In the future, we’ll likely need to expand the memory manager header beyond 64KiB in order to, for example, support a larger number of buckets. If that’s going to happen eventually, then there are simpler and more efficient ways to implement memory freeing. For example, we won’t need to store the bucket IDs as 15-bits, and we can use linked lists to make growing and freeing at O(# buckets), as opposed to O(max number of buckets) with the current approach.

There hasn’t been a lot of pressure on us to deliver this feature, so we can park this for now, close the ticket, and revisit with a more comprehensive design in the future.

@dragoljub-duric dragoljub-duric changed the title feat: Support memory freeing Draft: feat: Support memory freeing Nov 11, 2024
@dragoljub-duric dragoljub-duric changed the title Draft: feat: Support memory freeing feat: Support memory freeing Nov 11, 2024
@dragoljub-duric dragoljub-duric marked this pull request as draft November 11, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants