Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix formatting durations with fractions with leading zeroes #59

Closed
wants to merge 1 commit into from

Conversation

bens
Copy link
Collaborator

@bens bens commented Mar 12, 2024

The duration formatting function gives incorrect results for values with leading zeroes in the fraction component, for example I see this result:

benchmark              runs     total time     time/run (avg ± σ)     (min ... max)                p75        p99        p995      
-----------------------------------------------------------------------------------------------------------------------------
1 sphere               10       20.979s        2.97s ± 80.398ms       (2.7s ... 2.294s)            2.133s     2.70s      2.70s      

The total time is 20.9 seconds, but when divided by ten for the average it gives 2.9 seconds, not 2.09. Also see the min, max, and percentiles not making sense. This does change the output for cases like 1.0s to 1.000s, but I think it's better to show the precision in that case anyway.

Another issue was the division was always floored since it was using integer division, I've changed to using a float for the division down to a nice unit and using format's builtin rounding off at the time of printing.

Finally, for nanoseconds there's no actual fractional precision, so I added a special case for that to only show whole nanoseconds.

I've added tests for these cases as well of course.

@bens
Copy link
Collaborator Author

bens commented Mar 12, 2024

Output from the same benchmark after fixing the code makes a lot more sense:

benchmark              runs     total time     time/run (avg ± σ)     (min ... max)                p75        p99        p995      
-----------------------------------------------------------------------------------------------------------------------------
1 sphere               10       20.553s        2.055s ± 37.139ms      (2.015s ... 2.134s)          2.091s     2.064s     2.064s     

@bens bens force-pushed the format-fractions branch from 02a431c to 30c0ee3 Compare March 12, 2024 02:15
@bens bens force-pushed the format-fractions branch from 30c0ee3 to 7ee7530 Compare March 12, 2024 02:43
@@ -2,23 +2,21 @@ const std = @import("std");
const Color = @import("./color.zig").Color;
const log = std.log.scoped(.zbench_format);

pub fn duration(buffer: []u8, d: u64) ![]u8 {
pub fn duration(buffer: []u8, nanoseconds: u64) ![]u8 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, just using the stdlib's duration formatting is a good option too IMHO:

pub fn duration(buffer: []u8, ns: u64) ![]u8 {
    return try std.fmt.bufPrint(buffer, "{}", .{
        std.fmt.fmtDuration(ns),
    });
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Leveraging the std lib can reduce complexity and potential errors in this custom implementation.

@hendriknielaender
Copy link
Owner

@bens 🌟 Great work on your first contribution, and welcome to the project!

Thanks for adjusting the tests and fixing this.

@bens
Copy link
Collaborator Author

bens commented Mar 13, 2024

No idea why, but this PR seems to have become disconnected from my branch and isn't reflecting changes there. Will make a new one.

@bens bens closed this Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants