Closed 1996fanrui closed 3 weeks ago
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 theAmerica/New_York
case for Jiff. Do you know why?
https://github.com/BurntSushi/jiff/pull/104 is one reason, and I found another reason as well, I will prepare the PR later.
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.
Thanks @BurntSushi for the review and merge!
Some timezones have DST(daylight saving time) and others do not. For example:
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:
Note: I will submit some PR to optimize their performance later. (Merging this PR first is useful to observe the performance change.)