Skip to content

Use NonZeroUsize inside NodeId #17

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

Merged
merged 2 commits into from
Jan 18, 2019
Merged

Conversation

SimonSapin
Copy link
Contributor

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

To allow IDs to be used directly as indexes, the Vec<Node<T>> storage has an uninitialized item at index 0. Safety-wise, this "infects" the entire crate: every access to this Vec needs to make sure to skip that first (non-)item.

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

To allow IDs to be used directly as indexes,
the `Vec<Node<T>>` storage has an **uninitialized** item at index 0.
Safety-wise, this "infects" the entire crate:
every access to this `Vec` needs to make sure to skip that first (non-) item.
@SimonSapin
Copy link
Contributor Author

Safety-wise, this "infects" the entire crate

At first I started moving the Tree struct into a small module where the vec field would be private, so that this unsafety could be "more contained". But then I realize this module would need a non-trivial amount of abstraction in order to support the rest of the crate, particularly iterators. Still, at ~1k lines I feel that this crate is just above the size where this kind of pervasive unsafety is manageable, so maybe this abstraction would pay off. What do you think?

@SimonSapin
Copy link
Contributor Author

#16 was an alternative that offset IDs by 1 instead of having an uninitialized Vec item.

@causal-agent
Copy link
Collaborator

I'm not really happy with the amount of unsafe in this crate, but I'm not sure I'd be happier with creating more abstraction inside it either. At least for now, this looks fine. I'll reconsider if more changes need to be made in the future.

@causal-agent causal-agent merged commit 52169be into rust-scraper:master Jan 18, 2019
@SimonSapin SimonSapin deleted the nonzero branch January 18, 2019 19:06
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