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

Make iana-time-zone dep and local_tz function optional #178

Closed
lopopolo opened this issue May 10, 2023 · 4 comments
Closed

Make iana-time-zone dep and local_tz function optional #178

lopopolo opened this issue May 10, 2023 · 4 comments

Comments

@lopopolo
Copy link
Contributor

If strawlab/iana-time-zone#104 is merged, I'd like to depend on iana-time-zone directly so I can control which features are activated. I believe I'd be able to implement local_tz myself by calling:

let tz = tzdb::raw_tz_by_name(iana_time_zone::get_timezone()?);

Additionally, un-nesting iana-time-zone from this crate's dependency tree should improve compile times for apps depending on tzdb by allowing this crate (which takes 6 seconds to compile on my machine) to move earlier in the crate DAG.

@Kijewski
Copy link
Owner

I think the biggest "offender" is my macro

macro_rules! unwrap {
    ($($tt:tt)*) => {
        match $($tt)* {
            Ok(value) => value,
            Err(_) => {
                #[allow(unconditional_panic)]
                let err = [][0];
                err
            }
        }
    }
}

If I replace it with ↓, then the compile time is about 4 times faster in the simple test I did.

pub(crate) const fn time_zone_ref_new(
    transitions: &'static [tz::timezone::Transition],
    local_time_types: &'static [tz::LocalTimeType],
    leap_seconds: &'static [tz::timezone::LeapSecond],
    extra_rule: &'static Option<tz::timezone::TransitionRule>,
) -> TimeZoneRef<'static> {
    match TimeZoneRef::new(transitions, local_time_types, leap_seconds, extra_rule) {
        Ok(value) => value,
        Err(_) => {
            #[allow(unconditional_panic)]
            let err = [][0];
            err
        },
    }
}

I'll test it properly in the next few days.

Changing the default features would be a breaking change. But I could use breaking change this to increase the msrv from 1.56 to 1.57, so I could use panic!() in time_zone_ref_new(). I'll think about it. :)

@Kijewski
Copy link
Owner

Kijewski commented Dec 27, 2023

Please have a look if the path I took in tzdb = "0.6.0-pre.1" would work for you:

  • features = ["local"] enables function that use iana_time_zone.
  • If included, iana_time_zone is always used with feature = ["fallback"]

lopopolo added a commit to artichoke/artichoke that referenced this issue Dec 27, 2023
In Kijewski/tzdb#178, I asked upstream to make the dependency on
`iana-time-zone` optional so I could unnest the `iana-time-zone`
dependency in the cargo DAG.

This change was added in tzdb v0.6.0, which I am testing out the
pre-release for.

These changes require `spinoso-time` to depend on `iana-time-zone`
directly. `tzdb` is used without any features activated.

This change shaves about 1s off of builds times and moves
`iana-time-zone` and `tzdb` earlier in the cargo dep DAG as measured by
`cargo build --timings`.
@lopopolo
Copy link
Contributor Author

This works great! thanks @Kijewski 🙇

I've gone ahead and upgraded to tzdb v0.6.0-pre.1.

@Kijewski
Copy link
Owner

Thank you for testing the pre-release! :) I just pushed the stable release 0.6.0.

lopopolo added a commit to artichoke/artichoke that referenced this issue Dec 29, 2023
Now that the pre-release is merged.

Kijewski/tzdb#178 (comment)
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