dylanhart / ulid-rs

This is a Rust implementation of the ulid project
https://crates.io/crates/ulid
MIT License
381 stars 36 forks source link

[Proposal] use external base32 crate #34

Closed ngortheone closed 3 years ago

ngortheone commented 3 years ago

Would you be interested in a PR that replaces your in-house implementation of base32 with base32 crate

In my opinion this is better because:

So overall this is a net win: less code to maintain here, better for the ecosystem. What do you say?

dylanhart commented 3 years ago

I'd rather not use base32.

  1. It looks like using base32 would use Vec and String which would likely be a performance regression. (ulid only works on fixed sized strings, so dynamic allocation is never needed). It also wouldn't allow for this crate to ever become optionally no_std as I'm planning to eventually do. I'd be fine with dropping lazy_static by using a const function or [i8] (what base32 uses for the LUT) instead of [Option<u8>]. Dropping lazy static would allow the library to lose a dependency without gaining one.
  2. I'm all for adding better test coverage, but I think the better approach would be to run quickcheck tests (like what base32 has) on the entire library.
  3. Similar to 1, I'd rather drop lazy_static and provide a no_std build for reducing code bloat. When running cargo bloat on the cli, it looks like most of the code size is coming from some kind of sync primitive (lazy static?), but it's hard to tell since the cli has the same crate name. Needs more research.
$ cargo bloat -n 0 --release | grep ulid

    Analyzing target/release/ulid

 0.4%   0.6%   3.8KiB        ulid ulid::main
 0.2%   0.3%   1.9KiB        ulid std::sync::once::Once::call_once::{{closure}}
 0.1%   0.2%   1.3KiB        ulid core::cell::Cell<T>::replace
 0.1%   0.1%     768B        ulid ulid::base32::encode
 0.1%   0.1%     624B        ulid rand::rngs::adapter::reseeding::ReseedingCore<R,Rsdr>::reseed_and_generate
 0.1%   0.1%     624B        ulid ulid::Ulid::from_datetime_with_source
 0.1%   0.1%     544B        ulid <ulid::Ulid as core::fmt::Display>::fmt
 0.0%   0.1%     448B        ulid ulid::Opt::augment_clap::{{closure}}
 0.0%   0.1%     336B        ulid ulid::Generator::generate
 0.0%   0.0%     272B        ulid ulid::Opt::augment_clap::{{closure}}
 0.0%   0.0%     256B        ulid <ulid::Ulid as core::str::FromStr>::from_str
 0.0%   0.0%     224B        ulid ulid::Ulid::datetime
 0.0%   0.0%     144B        ulid alloc::raw_vec::RawVec<T,A>::reserve
 0.0%   0.0%     128B        ulid <ulid::base32::DecodeError as core::fmt::Display>::fmt
 0.0%   0.0%      96B        ulid <std::panicking::begin_panic::PanicPayload<A> as core::panic::BoxMeUp>::take_box
 0.0%   0.0%      64B        ulid ulid::Ulid::new
 0.0%   0.0%      48B        ulid std::panicking::begin_panic
 0.0%   0.0%      48B        ulid std::sys_common::backtrace::__rust_end_short_backtrace
 0.0%   0.0%      48B        ulid ulid::Ulid::to_string
 0.0%   0.0%      32B        ulid <std::panicking::begin_panic::PanicPayload<A> as core::panic::BoxMeUp>::get
 0.0%   0.0%      32B        ulid <&mut T as core::fmt::Display>::fmt
 0.0%   0.0%      32B        ulid core::ptr::drop_in_place
 0.0%   0.0%      32B        ulid core::ptr::drop_in_place
 0.0%   0.0%      32B        ulid core::ops::function::FnOnce::call_once{{vtable.shim}}
 0.0%   0.0%      16B        ulid core::ptr::drop_in_place
 0.0%   0.0%      16B        ulid <T as core::any::Any>::type_id
 0.0%   0.0%      16B        ulid ulid::Ulid::timestamp_ms
 0.0%   0.0%      16B        ulid ulid::Ulid::nil