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

WIP: DecayRange #486

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

WIP: DecayRange #486

wants to merge 2 commits into from

Conversation

mjp41
Copy link
Member

@mjp41 mjp41 commented Mar 21, 2022

Implemenation of a range that gradually releases memory back to
the OS. It quickly pulls memory, but the dealloc_range locally caches
the memory and uses Pal timers to release it back to the next level
range when sufficient time has passed.

  • codify that parent range needs to be concurrency safe.
  • Remove unused code

@mjp41 mjp41 force-pushed the decayrange branch 2 times, most recently from 5ad1005 to f7e897a Compare March 21, 2022 21:31

namespace snmalloc
{
template<typename Rep>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the reuse of the large buddy range rep here, at least a comment (or a concept) might be in order.

src/backend/decayrange.h Outdated Show resolved Hide resolved
src/backend/decayrange.h Outdated Show resolved Hide resolved
src/backend/decayrange.h Outdated Show resolved Hide resolved
}

// We have run out of memory.
handle_decay_tick(); // Try to free some memory.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be interlocked against the timer firing? I suppose not due to the prepend-only nature of all_local, the read-only nature of the spine traversal, and the use of pop_all for each found sizeclass... assuming that the parent range doesn't need interlocking, which, by default anyway, it doesn't (specifically, the parent will be a CommitRange whose parent is a GlobalRange by default, and CommitRange doesn't actually have state and GlobalRange is an interlock).

The presumption of concurrency-safeness of parent might merit being written down somewhere?

Copy link
Member Author

@mjp41 mjp41 Mar 22, 2022

Choose a reason for hiding this comment

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

Yeah, I plan to add a static constexpr to all the types that is the concurrency safe property like currently happens with Align. I just hadn't threaded it through yet. So GlobalRange would be true, CommitRange would be whatever the parent says, and the buddy would be false.

src/backend/decayrange.h Outdated Show resolved Hide resolved
@mjp41 mjp41 force-pushed the decayrange branch 6 times, most recently from f6254d6 to 93d6e3f Compare March 23, 2022 16:18
@mjp41
Copy link
Member Author

mjp41 commented Mar 23, 2022

So the perf of this is okay, but it increases memory footprint for some examples too much. I have factored out the primary changes to enable this into #491, so that can be landed, and the perf of this can be fixed and landed at a later point.

@mjp41 mjp41 force-pushed the decayrange branch 4 times, most recently from 37a1ce8 to 9694c96 Compare March 23, 2022 20:57
@mjp41
Copy link
Member Author

mjp41 commented May 13, 2022

This paper has a really interesting approach to work stealing of chunks between threads:
https://dl.acm.org/doi/10.1145/3533724

I think we could use some of the ideas in this paper, to make the decay range perform better.

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.

2 participants