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 support for custom measurement units #199

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andrewiggins
Copy link
Contributor

@andrewiggins andrewiggins commented Oct 16, 2020

This PR add support for defining measures with custom units. It defaults to ms.

I also renamed the millis field to rawData to avoid code assuming it refers to milliseconds (but if renaming that field is a breaking change we don't want, I can revert it).

Related: #181

@google-cla google-cla bot added the cla: yes label Oct 16, 2020
@andrewiggins
Copy link
Contributor Author

Build failure appears to be safari flaky-ness

@aomarks
Copy link
Member

aomarks commented Oct 26, 2020

Sorry for delay reviewing this!

It seems like there might be two goals here:

  1. Display measurements in an appropriately scaled unit for better readability.
  2. Allow measurement expressions to return units other than milliseconds.

For 1), this might be best suited as a responsibility of formatters, which could automatically pick the best unit to display for each measurement based on the values. +1 to doing this!

For 2), it seems like you could already add / 1000 to your JS expression to convert e.g. microseconds to milliseconds? Or are there some use cases I'm not thinking of?

I'm thinking we probably want to keep a single consistent unit in the internal and JSON representations of measurement values, because multiple units could lead to bugs/confusion down the road.

@andrewiggins
Copy link
Contributor Author

No worries :) I'm just circling back to this now lol

That's true, there are two concepts here. I was actually only concerned with 2) (though the examples in the tests I added definitely are better solved by 1) ). Specifically I was thinking about units other than time, for example memory (e.g. #181).

I was thinking that tachometer should only compare measurements of the same name/id. That way it can be agnostic to the unit that is reported and just trust that the measurements of the same name are in the same unit. This idea could work today with the performance and expression measurements which already have a name associated and since the callback measurement can only be used once, it can be assumed that all the callback based measures in a bench run are measuring the same thing.

Though if you are thinking tracking memory is currently out of scope for tachometer, I understand. We can currently get by with what exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants