bench: add the zoned benchmark for Asia/Shanghai

[1996fanrui]

Some timezones have DST(daylight saving time) and others do not. For example:

• "America/New_York" time zone has DST
• "Asia/Shanghai" had DST a long time ago, but it was cancelled long ago.

When I look into Zoned::new, I found the code path with DST is very different from the code path without DST. And their performance is totally different.

So I propose to improve the instant_to_civil_datetime_static to bench DST and Non-DST separately.

Here is the benchmark result on my Mac:

cargo bench --bench jiff-bench
critcmp -g '^[^/]+/(.*)$' -f '^(chrono|chrono-tzfile|jiff)/' base

group                                                base/chrono-tzfile/                    base/chrono/                           base/jiff/
-----                                                -------------------                    ------------                           ----------
instant_to_civil_datetime_static-America/New_York    1.19     46.2±1.01ns        ? ?/sec    1.00     38.8±0.98ns        ? ?/sec    1.92     74.6±2.27ns        ? ?/sec
instant_to_civil_datetime_static-Asia/Shanghai       1.00     34.7±0.51ns        ? ?/sec    1.08     37.4±0.62ns        ? ?/sec    2.87     99.3±1.35ns        ? ?/sec

Note: I will submit some PR to optimize their performance later. (Merging this PR first is useful to observe the performance change.)

[BurntSushi]

Thanks!

For the future, a good commit hygiene in changes like this would be to have one commit that only does the refactor and then a second commit that introduces the new benchmark. It makes review easier. No need to do that now for this PR though.

The benchmark results show that the Asia/Shanghai case is slower than the America/New_York case for Jiff. Do you know why?

[BurntSushi]

Please follow the style of the surrounding code. Move the use statements to the top of the function and combine them into one.

[BurntSushi]

Can you use Tz::from_str_insensitive instead?

[BurntSushi]

Assuming you use Tz::from_str_insensitive, I don't think you'll need named constants any more.

[BurntSushi]

This function should probably be defined within instant_to_civil_datetime_static. And since the scope is narrower there, you can just call it define.

[1996fanrui]

you can just call it define.

Sorry, I don't understand it. Do you mean rename the funcation name to instant_to_civil_datetime_static_define?

[BurntSushi]

No. define. But you need to make it an inner function first:

fn instant_to_civil_datetime_static(...) {
    fn define(...) {
        ...
    }
    define("America/New_York", ...);
    define("Asia/Shanghai", ...);
}

[1996fanrui]

Thank you for the quick review!

For the future, a good commit hygiene in changes like this would be to have one commit that only does the refactor and then a second commit that introduces the new benchmark. It makes review easier. No need to do that now for this PR though.

Got it. I'm curious, does jiff use squash merge(one commit) or rebase merge(keep multiple commits) after PR is reviewed?

If rebase merge(keep multiple commits), I could re-orginaze commits during I address your comment.

The benchmark results show that the Asia/Shanghai case is slower than the America/New_York case for Jiff. Do you know why?

#104 is one reason, and I found another reason as well, I will prepare the PR later.

[BurntSushi]

Got it. I'm curious, does jiff use squash merge(one commit) or rebase merge(keep multiple commits) after PR is reviewed?

I use both strategies. It's difficult to articulate succinctly when I use them. For my own PRs, I almost always use rebase & merge because I craft the commits to be suitable for that purpose. For contributor PRs, commit hygiene usually isn't a focus and the changes tend to be small, so I squash & merge them. In rare cases of good commits from contributors, then I'll rebase and merge (usually after improving the commit messages locally). In other rare cases, I'll sometimes rewrite PRs to use multiple commits if I think the history would meaningfully benefit from it.

For this change, which is pretty small, I'll very likely use squash and merge regardless of what you do. Please do not spend time reorganizing this PR into commits. I mentioned it for future PRs. Separating changes like "refactor but don't change behavior" from "change behavior" makes review easier, even if I ultimately still squash & merge.

[1996fanrui]

Hey @BurntSushi , I have addressed all your comments, please help take a look in your free time, thanks a lot~

[1996fanrui]

Thanks @BurntSushi for the review and merge!