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

[Proposal] use external base32 crate #34

Closed
ngortheone opened this issue Dec 15, 2020 · 1 comment
Closed

[Proposal] use external base32 crate #34

ngortheone opened this issue Dec 15, 2020 · 1 comment

Comments

@ngortheone
Copy link

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:

  • that base32 crate has zero dependencies, this allows to drop lazy_static dependency
  • slightly better test coverage
  • most-importantly standalone - that reduces the code bloat if a downstream project uses both this and base32 crate

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

@dylanhart
Copy link
Owner

dylanhart commented Dec 15, 2020

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

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

No branches or pull requests

2 participants