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

Allow nodes to be RCed and deallocated from the arena #11

Open
kneasle opened this issue Nov 19, 2020 · 3 comments
Open

Allow nodes to be RCed and deallocated from the arena #11

kneasle opened this issue Nov 19, 2020 · 3 comments
Labels
help wanted Extra attention is needed

Comments

@kneasle
Copy link
Owner

kneasle commented Nov 19, 2020

Currently, Sapling will never free up space in the arena even if history is rewritten and some trees will never be reached. This is essentially a memory leak, and will cause Sapling to just accumulate memory over time.

Some kind of garbage collection has to happen, because Sapling's arena allocator only produces immutable references to the nodes. However, because of the tree structure of the AST, we can rely on AST nodes having no reference cycles and so standard reference counting should suffice to prevent memory leaks. However, some custom drop code will be required (because we can't simply deallocate small parts of memory owned by the Arena - we need to notify the arena that this cell isn't being used).

So I think the solution is going to have to involve some kind of smart pointer that does reference counting but, instead of deallocating the memory it points to, instead notifies the Arena whenever the reference count hits 0. Also, AST nodes in the arena currently sit inside Item structures (which in its own right probably should be called Cell), which allows us to add extra fields for things like RC and memoisation of expensive functions (string conversions, etc.).

My proposed solution would be something like having a Ref<'arena, Node> smart pointer, which contains an RCed reference to an arena::Item<Node>, which can then be deref-ed into just a plain &'arena Node.

Node memory management is definitely not something that I want to screw up, and I've never had to deal with smart pointers or RCing before so if someone who knows more is happy to implement this then feel free :).

@kneasle
Copy link
Owner Author

kneasle commented Nov 19, 2020

@Kixunil would you be happy doing this?

@kneasle kneasle added the help wanted Extra attention is needed label Nov 19, 2020
@Kixunil
Copy link
Contributor

Kixunil commented Nov 21, 2020

I'm not very familiar with the internals yet and I'm lacking time for a bigger contribution lately. I can reconsider later.

@kneasle
Copy link
Owner Author

kneasle commented Nov 21, 2020

OK that's fine - I was just wondering if you wanted too.... this will only really become useful when Sapling gets more functionality and people actually use it for extended periods of time, so there's not really much hurry to risk breaking things until needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants