Skip to content

audit jiff::fmt::strtime::BrokenDownTime API #495

@BurntSushi

Description

@BurntSushi

There are some possible performance footguns in the design of the BrokenDownTime type that could prevent Jiff from doing the fastest possible thing. There is some tension between API simplicity and borrowing data via a lifetime parameter.

The two obvious candidates for borrowing are a Zoned's TimeZone value and a Zoned's IANA time zone identifier. It also seems like these two things could be merged, but IIRC, the IANA time zone identifier is a separate field to support parsing identifiers without necessarily eagerly turning it into a TimeZone. But maybe that's not worth doing.

In any case, we could explore adding a lifetime parameter to a BrokenDownTime. Today it has none. And if we did this, we'd likely need to put them inside Cow wrappers and add routines for "borrowing" or "owning" a BrokenDownTime. This pattern is actually already used by Jiff in other less prominent places in the API. It can also be found in the memchr crate.

I don't really like doing this because I think lifetime/type parameters add cognitive overhead to understanding an API. It's precisely why Jiff is very light on such things already. And in the case of BrokenDownTime, there are likely only some things that will be borrowed (the data that is non-Copy, which can only come from a Zoned value), which makes it... somewhat weird. With that said, it is somewhat rare to need to interact with a BrokenDownTime directly, and it is considered a lower level API. So I'm definitely open to this.

I really do not want to add two lifetime parameters though (one for tz and one for iana). If we didn't merge them, we could of course insist that they share a lifetime. Which is... probably reasonable.

One thing I haven't thought too much about is the mutator APIs. Right now, for example, BrokenDownTime::set_iana_time_zone mutates the value in place. I think this is still doable with a lifetime parameter, but I'd want to play around with it. We could alternatively only provide the mutator API on impl BrokenDownTime<'static>.

See #494 for an attempt at a change like this and some ensuing discussion.

Metadata

Metadata

Assignees

No one assigned

    Labels

    breaking changeIssues that require a breaking change for resolution.enhancementNew feature or request

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions