Skip to content

Use NonZeroUsize inside NodeId #16

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

Closed
wants to merge 2 commits into from

Conversation

SimonSapin
Copy link
Contributor

This allows Option<NodeId> to be no larger than usize, and saves 32 bytes of memory per node.

This allows `Option<NodeId>` to be no larger than `usize`,
and saves 32 bytes of memory per node.
@SimonSapin
Copy link
Contributor Author

This library looks really nice! This PR is a memory use improvement that you might be interested in.

It adds some CPU instructions for those + 1 and - 1, but hopefully that cost is small enough. A way to avoid that could be to have a dummy unused node at index zero. But then we’d have to come up with a dummy T value for it (requiring T: Default just for this seems unfortunate), or unsafely leave it uninitialized.

In the same spirit, it looks like NodeRef could also be made smaller by replacing its node field by a node() method similar to that of NodeMut. Since indexing is unchecked, I think the only cost would be the extra pointer indirection (NodeRef.treeTree.vec.ptrNode v.s. NodeRef.nodeNode). NodeRef may seem small already (relatively to, say, Node<T>) but in code that passes it around a lot, those copies/moves can add up.

@SimonSapin
Copy link
Contributor Author

But then we’d have to […] unsafely leave it uninitialized.

On further thought this isn’t so bad, with Vec::with_capacity + set_len. Let me do that instead :)

@causal-agent
Copy link
Collaborator

It's been ages but I'd like to merge this one since it's much less code and more convincingly correct. The branch is gone however so I can't reopen this PR, but I'm guessing I can grab the commits from GitHub some way.

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