servo / uluru

A simple, fast, LRU cache implementation.
Mozilla Public License 2.0
190 stars 20 forks source link

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

Open rmsyn opened 8 months ago

rmsyn commented 8 months ago

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

mbrubeck commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.