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

Add script to convert units to a base value. #48

Merged
merged 8 commits into from
Jan 14, 2025
Merged

Conversation

dvalinrh
Copy link
Contributor

@dvalinrh dvalinrh commented Nov 20, 2024

We need a tool that given a unit value and value, will convert to another unit. This is required because

  1. Multiple runs of the same benchmark can results in different units being provided.
  2. The ability to show the results of a benchmark always in the same unit.
  3. This spans multiple benchmarks, so a generic solution is required.

Given a unit, convert it to the base.
Time base is nano seconds
Memory base is bytes

There is an existing program, numfmt that provides the units for us, so instead of duplicating each functionality we will leverage it. Also we do not want to duplicate the required logic in every wrapper, so we create a generic script.

To differentiate between time and a 10 based measurement we will symlink a well known script name that is for time. We check $0 on execution of the script if it is the time symlink, then we know we are dealing with time values, and act accordingly.

@dvalinrh dvalinrh requested a review from a team November 20, 2024 12:57
unit_conversion Outdated Show resolved Hide resolved
unit_conversion Outdated Show resolved Hide resolved
unit_conversion Outdated Show resolved Hide resolved
convert_numeric Outdated Show resolved Hide resolved
convert_numeric Outdated Show resolved Hide resolved
convert_numeric Outdated Show resolved Hide resolved
convert_numeric Outdated Show resolved Hide resolved
Copy link
Member

@kdvalin kdvalin left a comment

Choose a reason for hiding this comment

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

I think there's a better way to differentiate between time conversion and normal unit conversion than what the running script name is.

convert_numeric Outdated Show resolved Hide resolved
convert_numeric Outdated Show resolved Hide resolved
1) Only one program, use the option --time_val instead of symlink
2) Change options to be --from_unit --to_unit.
convert_val Show resolved Hide resolved
Copy link
Member

@kdvalin kdvalin left a comment

Choose a reason for hiding this comment

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

Minor nits

convert_val Outdated Show resolved Hide resolved
convert_val Show resolved Hide resolved
convert_val Outdated Show resolved Hide resolved
convert_val Show resolved Hide resolved
convert_val Show resolved Hide resolved
@dvalinrh dvalinrh requested a review from kdvalin January 14, 2025 14:30
Copy link
Member

@kdvalin kdvalin left a comment

Choose a reason for hiding this comment

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

LGTM

@dvalinrh dvalinrh merged commit b6d5622 into main Jan 14, 2025
6 checks passed
@dvalinrh dvalinrh deleted the add_unit_convert branch January 14, 2025 15:59
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.

3 participants