Consuming or mutable `add` shouldn't clone the timezone

[Nahor]

(jiff v0.2.18)

Some functions, such as Zoned::add(self) and Zoned::add_assign(&mut self) are expressed in terms of Zoned::add(&self). The later necessarily requires the creation of a new Zoned object, followed by the drop of the old self (either by assignment, or by returning from the function).

In particular, this means that the original TimeZone must be cloned/copied first (for the new Zoned), then dropped. In the case of ARC_TZIF or ARC_POSIX, this involves a relatively costly atomic operations (e.g. #271). According to a flamegraph, in my case it accounted for half the cost of the add():

(in fushia are the entries containing the word)

Disclaimer: my test was running in a highly artificial scenario, with lots of threads, on lots of CPU cores, mostly doing Zoned::adds, so I suspect a lot of the cost is due to contention/synchronization on the Arc's atomic counter rather than the pure arithmetic on it.
Still, since the old TimeZone will not be used anymore, it shouldn't be necessary to clone the TimeZone and instead it could be moved.

In other words, it would be better if add(&self) was expressed in terms of add(self) (i.e. self.clone().add(...)), instead of the other way around.

[BurntSushi]

I would be more interested in a non-artificial benchmark. Do you have a real work load that suffers here? And would this problem actually be solved by your suggestion? (I can't tell because I don't see any code offered here.)

There are already consuming Add impls (among others), but they don't avoid cloning the timezone. I would be open to a patch optimizing that specific case.

Otherwise, I'm not inclined to change the API such that callers are required to do an explicit clone for every addition. That seems like a cure worse than the disease.

I think the solution here is to expose lower level APIs that provide the Zoned operations in a context that only needs a &TimeZone. Callers that need to avoid the arc clone can then use those and Zoned can be called a higher level convenience API.

Otherwise I don't really see a feasible way to fix the problem. Artificial benchmarks don't really help IMO.

[BurntSushi]

Brainstorming: what if we added an opt-in crate feature that specifically leaks every time zone it reads from disk (and also will never read that time zone from disk again)? In that mode, I could arrange it such that clones were cheap. In exchange, you might have worse memory usage (although not true in the status quo because a TimeZoneDatabase will never drop a time zone unless it is re-read from disk, in which case, there's no decrease in memory usage) and you for go automatic updates to the tzdb on disk (instead you're required to restart the application).

I think I can also arrange it such that if you are using time zones from jiff-tzdb, then those clones will be cheap as well. This would at least match what you get with chrono-tz.

There's a fair bit of work required to do this, but I was planning on doing much of it anyway.

[Nahor]

I would be more interested in a non-artificial benchmark

Just to be clear, when I said "artificial", I didn't mean built-to-show-the-problem. I meant that I don't expect it to be a common scenario. I guess "niche" would be a more accurate term.

I found that issue when I converted from chrono to jiff and noticed that jiff was significantly slower and investigated it.
(I can get that speed back and then some by using the TimeZone::get! macro, i.e. getting of the Arc, but I'm not keen in hardcoding the timezone, this is a recipe for future bugs).

I'm not inclined to change the API such that callers are required to do an explicit clone for every addition.

I don't think the API needs to be changed. Worst case, one would need to add a consuming version of some of the API (e.g. Zoned::checked_add_consuming(self, duration)). Alternatively, it would be to move the functions into traits (e.g. trait CheckedAdd { fn checked_add(Self, Duration); impl CheckedAdd for Zoned {...}; impl CheckedAdd for &'a Zoned {...}).
For the rest, it's just swapping what function calls what, i.e. instead of Zoned::add(self, ...) { (&self).add(...) } have Zoned::add(&self, ...) { self.clone().add(...) }

And would this problem actually be solved by your suggestion?

It would, necessarily, since the issue is cloning the Arc and the change would use a move instead.

also will never read that time zone from disk again

Honestly, I think that should be standard, unless explicitly requested by the user. Because as-is, if the tz changes on disk, two Zoned with the same timestamp and "same zone" (based on name) may actually have different dates, and that would be surprising and source of bugs (I doubt many people test their code for changing tz).
(Alternatively, it would be to have TimeZone be internally mutable, i.e. an update would affect all instances, but that would be quite costly, more than Arc, since rwlocks would be needed)

I can also arrange it such that if you are using time zones from jiff-tzdb, then those clones will be cheap as well

That would be a nice improvement and help in other cases (like when creating a new Zoned). But I see it more as a separated improvement, which happens to also improve Add and AddAssign, without actually fixing the issue. Even if it become cheaper, the cloning is still there and is still unnecessary.
And it wouldn't fix the case of the system timezone (unless it can be mapped to a jiff-tzdb?)

[BurntSushi]

Hmmm ok.

Adding traits for solving a problem like this is way overkill. I'd sooner add a consuming Zoned::checked_add, but that has problems too. There are lots of other methods that would want this treatment (e.g., Zoned::start_of_day) and adding consuming versions of all of those does not sound like a good API user experience. Moreover, most such calls are fallible, which means ideally the error type provides a way to get the original Zoned value back. That complicates the API even more.

But yes, like I said, I am at least open to optimizing the impl Add<...> for Zoned case (among the other operations).

It would, necessarily, since the issue is cloning the Arc and the change would use a move instead.

If you were using Chrono before this, then are you using a std::time::Duration or jiff::SignedDuration? If so, the time zone doesn't really come into play during checked_add. So you could do the arithmetic on the underlying Timestamp instead. It's unfortunate you can't "just use" Zoned, but I suggest this as a work-around for now.

Honestly, I think that should be standard, unless explicitly requested by the user. Because as-is, if the tz changes on disk, two Zoned with the same timestamp and "same zone" (based on name) may actually have different dates, and that would be surprising and source of bugs (I doubt many people test their code for changing tz).

Can you say more about what specific bug could occur? Note that if you for example try to subtract two Zoned values in a way that would produce calendar units but your time zones are different, you'll get an error:

jiff/src/zoned.rs

Lines 4308 to 4310 in 66ba0fe

	if zdt1.time_zone() != zdt2.time_zone() {
	return Err(Error::from(E::MismatchTimeZoneUntil { largest }));
	}

So if you somehow have two Zoned values with the same IANA time zone identifier but with different time zone transitions, then you'll get an error if you try to do something with those two values in a way that could be impacted by the differing time zone transitions.

In any case, there are other reasons why this shouldn't, IMO, by the default:

• Leaking data tied to user input is bad juju. While today Jiff will never automatically free TimeZone values inside of a TimeZoneDatabase, users can call TimeZoneDatabase::reset to do just that. If we leak the time zones, this API becomes impossible to implement.
• When I designed this architecture, my research revealed that some users expect datetime libraries with time zone integration to automatically use updated tzdb data without needing to restart the application. This seems like the better default because it "just does" the right thing for you.
• Users may use other TimeZoneDatabase values beyond the global. While users cannot drop the global TimeZoneDatabase, they can drop databases they create. It would be bad juju to not be able to free TimeZone values produced by that database. (I think this could be mitigated by only applying the leak behavior to the global TimeZoneDatabase.)

That would be a nice improvement and help in other cases (like when creating a new Zoned). But I see it more as a separated improvement, which happens to also improve Add and AddAssign, without actually fixing the issue. Even if it become cheaper, the cloning is still there and is still unnecessary.

I don't think it's separate. The issue here isn't the cloning. The issue here is expensive cloning. You even said yourself that if you switch to jiff::tz::get! the performance problem goes away (and even improves compared to chrono). So I would consider this a solution to the problem here. Cloning a leaked time zone should just be copying a pointer, so in that case we can call it "effectively free."

And it wouldn't fix the case of the system timezone (unless it can be mapped to a jiff-tzdb?)

It would. In Jiff, the system time zone isn't usually special. The only special bit is discovering the system configured IANA time zone identifier. After that point, it's just a normal tzdb lookup.

The one case that wouldn't be fixed is when, e.g., there is an /etc/localtime in a Unix environment but in a way where the IANA time zone identifier couldn't be discovered. Jiff will still use the time zone transitions in /etc/localtime by reading it directly, outside the context of a TimeZoneDatabase. This results in a degraded experience because there's no real way to serialize zoned datetimes using this time zone in a non-lossy way because there is no IANA time zone identifier. Since this is generally a configuration error IMO, having this also result in more expensive clones seems fine to me.

But we could always choose to leak the system time zone as well (in the one specific case where an IANA tz identifier could not be found) and stipulate that it will not be re-read from disk after this point.

[BurntSushi]

I put up #492 with the narrow suggestion here (don't clone TimeZone for consuming APIs) and included an ad hoc benchmark program too: #492 (comment)

[Nahor]

[Just for the record since you merged a fix already]

adding consuming versions of all of those does not sound like a good API user experience

I wasn't advocating to add consuming versions for everything. The idea of checked_add_consuming is not to have a consuming checked_add per-se, but to support the existing consuming add (and even more so add_assign since one of the big points of mutable operations is to avoid temporary objects).
start_of_day is not used by any consuming/inplace functions as far as I can tell, so I think it's fine to keep it as-is (until someone finds that it's a problem that is).

I suggest this as a work-around for now.

My work-around is to use the TimeZone::get! macro since that also solve the creation of new Zoned. But I didn't file the issue because I was stuck, I did it because I felt it was a "bug" (not in the sense that it was broken either, but in the sense that the code was doing something it shouldn't be doing, i.e. unexpected given the consuming/inplace API).

Can you say more about what specific bug could occur?

If you're asking for a real, non-theoretical bug, then I do not have one. If you're OK with a theoretical but highly plausible: a calendar application trying to find overlapping appointments. I would think a lot of people assume the following will always be true:

    assert_eq!(
        datetime(2026, 7, 1, 0, 0, 0).in_tz("America/Los_Angeles"),
        datetime(2026, 7, 1, 0, 0, 0).in_tz("America/Los_Angeles")
    );

but as I understand it, this would fail if the file changes in between the two in_tz calls. I expect that even quite a few people who want updated timezones to still be surprised by the failure, because they would expect/want the timezones to be updated globally (although that adds other issues, like the tz getting updated in the middle of call to PartialEq::eq(), but that's outside the point I'm trying to make).

my research revealed that some users expect datetime libraries with time zone integration to automatically use updated tzdb data without needing to restart the application. This seems like the better default because it "just does" the right thing for you.

I have no doubt that some users want that. However:

• How many of them want updates on a per-timezone-instance basis, as opposed to a global/update-all-existing-instances? I guess a lot, if not most, will not care either way because they don't actually have long-lived Zoned, or don't need to compare two Zoned, but among the remaining, I expect most would rather have the latter category (gut feeling).
• "does the right thing" is very contextual. I'm sure that for some other people, the "right thing" is to not update without warning/explicit consent.

The issue here isn't the cloning. The issue here is expensive cloning.

I disagree. If cloning was required (like for add(&self)), then I would agree with you. But since cloning is not required (and even arguably unexpected in the case of add_assign), to me, the expensive cloning is only a symptom, and the source/core of the problem is the cloning. Making cloning cheaper makes the problem unnoticeable, sure, but there would still be some unnecessary cost (albeit very small).