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

deps: use heapless::Vec for fixed-capacity Vec #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rmsyn
Copy link

@rmsyn rmsyn commented Nov 12, 2023

Switches to the actively maintained heapless crate for a no_std-compatible, fixed-capacity Vec implementation.

Switches to the actively maintained `heapless` crate for a
`no_std`-compatible, fixed-capacity `Vec` implementation.
@mbrubeck
Copy link
Collaborator

What are the concrete advantages of using heapless over arrayvec for this crate?

I see that arrayvec maintenance has indeed fallen behind. However, this crate only needs the most basic features of arrayvec, which I consider to be fairly stable and well-tested. Depending on heapless means we also need to build other crates (currently byteorder, hash32, and stable_deref_trait), all with separate maintainers. It increases the total amount of code in the uluru dependency tree by almost an order of magnitude, more than doubling the build time on my slow laptop, and making code audits more arduous.

This would somewhat bloat the Firefox and Servo builds, for example, which already have other dependencies on arrayvec but not on heapless.

I'm not opposed to this change, especially if lack of maintenance of arrayvec is causing practical problems. But I want to make sure the trade-offs are correct.

@rmsyn
Copy link
Author

rmsyn commented Nov 12, 2023

What are the concrete advantages of using heapless over arrayvec for this crate?

My main thoughts are around heapless being a maintained crate, authored/maintained by rust-embedded community members. This crate is likely to see much more attention than arrayvec into the future.

Depending on heapless means we also need to build other crates (currently byteorder, hash32, and stable_deref_trait), all with separate maintainers. It increases the total amount of code in the uluru dependency tree by almost an order of magnitude

I agree that the dependency graph and build times increasing that much adds burden. Considering that uluru usage of the fixed-size ArrayVec is actually very simple, a rewrite of that functionality is another alternative. Something like a thin wrapper:

pub struct LruVec<T, const N usize> {
  arr: [T; N],
}

impl<T, const N: usize> LruVec<T, N> {
...
}

From first glance, there's only a handful of methods to implement. I can look into doing the work. Is this something you would like? It would have the benefits of dropping a dependency, and likely decrease compile times.

A possible downside would be if other parts of Servo/Firefox wanted to use LruVec, and they needed functionality closer to the full features of arrayvec. Which would make forking arrayvec a better solution.

@mbrubeck
Copy link
Collaborator

Another option is to use optional dependencies to let users choose between the arrayvec and heapless implementations. This could be useful if they are integrating uluru into a project that already contains one or the other of these dependencies...

@rmsyn
Copy link
Author

rmsyn commented Nov 12, 2023

Another option is to use optional dependencies to let users choose between the arrayvec and heapless implementations.

This might be the best compromise.

Also, the only required dependency in heapless is hash32, and that dependency won't get compiled into a project that doesn't use the serde feature, indexmap, and/or indexset modules. Not sure why you are experiencing such an increase in build times.

Even when compiling hash32, its only dependency is byteorder, which has no dependencies. So, at most, the dependency graph grows by two crates. Both of which are well-maintained, though hash32 hasn't been updated since last year.

@mbrubeck
Copy link
Collaborator

Also, the only required dependency in heapless is hash32,

heapless requires both stable_deref_trait and hash32, and hash32 requires byteorder.

The increase in compile time is small in absolute terms (less than 1 second of wall clock time), but it illustrates the change in total amount of code downloaded and compiled.

@rmsyn
Copy link
Author

rmsyn commented Nov 12, 2023

The increase in compile time is small in absolute terms (less than 1 second of wall clock time), but it illustrates the change in total amount of code downloaded and compiled.

I honestly think that is worth the gained advantage of using a well-maintained dependency. There may also be some possibility of also making hash32 and stable_deref_trait optional dependencies.

Not familiar enough with the internals of heapless to say for sure.

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