Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Benchmark improvements #38

Closed
jhpratt opened this issue Jul 24, 2024 · 2 comments · Fixed by #44
Closed

Benchmark improvements #38

jhpratt opened this issue Jul 24, 2024 · 2 comments · Fixed by #44
Labels
bug Something isn't working

Comments

@jhpratt
Copy link

jhpratt commented Jul 24, 2024

Some things that I noticed while looking through the benchmarks:

  • Why assert_eq! on everything? Wouldn't let _ = black_box(foo); be sufficient for the compiler?
  • Related to the previous, there's no need to calculate the Unix timestamp when checking for equality for either chrono or time. The values can be compared directly, avoiding an unrelated computation.
  • parse_civil_datetime can use the well-known ISO 8601 format for time, which should be significantly faster. Otherwise it's using the same parsing mechanism as your strptime test.
@BurntSushi
Copy link
Owner

Why assert_eq! on everything? Wouldn't let _ = black_box(foo); be sufficient for the compiler?

My philosophy on benchmarks is that they should have some kind of verification step that ensures they are arriving to the expected answer. I've been burned way too many times by benchmarks measuring something you don't expect because the result isn't checked.

Related to the previous, there's no need to calculate the Unix timestamp when checking for equality for either chrono or time. The values can be compared directly, avoiding an unrelated computation.

There is one benchmark where this is intentional (offset_to_instant), and I'll rename that to offset_to_timestamp to better reflect the intent of the measurement. But you're right, the other benchmarks where I'm asking for a Unix timestamp from Chrono and time are wrong. I'll fix those. Likely a copy & paste related casualty.

parse_civil_datetime can use the well-known ISO 8601 format for time, which should be significantly faster. Otherwise it's using the same parsing mechanism as your strptime test.

Oh nice catch! Yes, I'll fix this.

@BurntSushi BurntSushi added the bug Something isn't working label Jul 25, 2024
BurntSushi added a commit that referenced this issue Jul 25, 2024
There were a few benchmarks where I was doing `timestamp()` comparisons
unnecessarily. It was probably a transcription error. There is one
benchmark though where it is intentional.

Also, we used the `well_known` ISO 8601 format for `time` when parsing
civil datetimes. There is a significant speed-up that comes with it.
It's a hair faster than Jiff now in my benchmarks.

Fixes #38
@jhpratt
Copy link
Author

jhpratt commented Jul 26, 2024

Yeah, I was referring to the benchmarks where it wasn't the intent. I recognized that one was specifically for that.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants