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 option to partition results table by measurements #198

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

Conversation

kevinpschaaf
Copy link
Member

@kevinpschaaf kevinpschaaf commented Oct 9, 2020

Fixes #197

collate: false partition: undefined:
image

collate: true partition: "measurement":
image

@google-cla google-cla bot added the cla: yes label Oct 9, 2020
@kevinpschaaf kevinpschaaf marked this pull request as ready for review October 9, 2020 23:29
@kevinpschaaf kevinpschaaf requested a review from aomarks October 9, 2020 23:29
@kevinpschaaf
Copy link
Member Author

@aomarks I can't seem to get the Travis tests to re-run (it looks like a Safari flake in an unrelated test?)

Copy link
Member

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

Looks good, just a comment about the name.

CHANGELOG.md Outdated
@@ -7,6 +7,9 @@ project adheres to [Semantic Versioning](http://semver.org/).

<!-- ## Unreleased -->

- Add `collate` option for collating results tables by measurement (when
Copy link
Member

Choose a reason for hiding this comment

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

How about partition: "measurement"?

I also thought about names compare and group-by, but I'm leaning towards partition because it seems to best describe "the thing you want to do to the table after you see that it's too big".

Also I'm thinking the value should be measurement instead of true so that we have more room to expand with more partitioning options in the future.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's nice. So conceptually we could add something like partition: "browser" to partition the results table based on browser? And next level, make it an array to partition on multiple dimensions, i.e. browser and measurement?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. Was out yesterday but will try and get an update up soon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've addressed the feedback. All instances of collate: true have been replaced by partition: "measurement".
I also added documentation to the README for the new option.

Copy link
Contributor

@andrewiggins andrewiggins left a comment

Choose a reason for hiding this comment

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

Love this! 🎉

CHANGELOG.md Outdated
@@ -7,6 +7,9 @@ project adheres to [Semantic Versioning](http://semver.org/).

<!-- ## Unreleased -->

- Add `collate` option for collating results tables by measurement (when
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's nice. So conceptually we could add something like partition: "browser" to partition the results table based on browser? And next level, make it an array to partition on multiple dimensions, i.e. browser and measurement?

@@ -13,6 +13,7 @@
"scripts": {
"prepare": "if [ -f './tsconfig.json' ]; then npm run build; fi;",
"build": "rimraf lib/ client/lib/ && mkdir lib && npm run generate-json-schema && tsc && tsc -p client/ && npm run lint",
"build:watch": "tsc --watch",
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call!

src/format.ts Outdated
@@ -104,6 +105,23 @@ export function automaticResultTable(results: ResultStats[]): AutomaticResults {
return {fixed: fixedTable, unfixed: unfixedTable};
}

export function collatedResultTables(results: ResultStatsWithDifferences[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be any value in skipping the difference computation for dimensions that are partitioned? Though I guess that is larger breaking change that impacts more parts of the code.... Perhaps something to explore later?

@marvinhagemeister
Copy link

Love this! It is a huge improvement for readability of the results 🙌

@marvinhagemeister
Copy link

Still interested in having this. What are the open tasks to get this in?

@prudepixie
Copy link

Hi there, are there any plans to get this merged? Very useful and would like to use it in our benchmark set up

@prudepixie
Copy link

Hi there, are there any plans to get this merged? Very useful and would like to use it in our benchmark set up

@aomarks 🥇

@AndrewJakubowicz AndrewJakubowicz changed the title Add option to collate results tables by measurements Add option to partition results table by measurements Nov 28, 2023
src/config.ts Outdated Show resolved Hide resolved
@AndrewJakubowicz
Copy link
Collaborator

AndrewJakubowicz commented Nov 30, 2023

I've found an edge case for small results tables that can't be partitioned. E.g. ./bin/tach.js --config lit-element/list/tachometer.json, where ./bin/tach.js is tachometer built from this PR. This returns:

┌─────────────┬─────────────────┐
│     Version │ <none>          │
├─────────────┼─────────────────┤
│     Browser │ chrome-headless │
│             │ 119.0.6045.199  │
├─────────────┼─────────────────┤
│ Sample size │ 50              │
├─────────────┼─────────────────┤
│       Bytes │ 30.95 KiB       │
└─────────────┴─────────────────┘

┌───────────────────┐
│          Avg time │
├───────────────────┤
│ 19.81ms - 20.64ms │
└───────────────────┘

┌─────────────────────┐
│            Avg time │
├─────────────────────┤
│ 202.71ms - 205.71ms │
└─────────────────────┘

┌─────────────────────┐
│            Avg time │
├─────────────────────┤
│ 196.53ms - 198.90ms │
└─────────────────────┘

This has been partitioned from:

┌──────────────────────────────┬─────────────────────┬─────────────────────────┬─────────────────────────┬─────────────────────────────────┐
│ Benchmark                    │            Avg time │ vs this-change [render] │ vs this-change [update] │ vs this-change [update-reflect] │
├──────────────────────────────┼─────────────────────┼─────────────────────────┼─────────────────────────┼─────────────────────────────────┤
│ this-change [render]         │   20.32ms - 21.24ms │                         │                  faster │                          faster │
│                              │                     │                -        │               90% - 90% │                       89% - 90% │
│                              │                     │                         │     183.28ms - 187.37ms │             177.64ms - 181.91ms │
├──────────────────────────────┼─────────────────────┼─────────────────────────┼─────────────────────────┼─────────────────────────────────┤
│ this-change [update]         │ 204.11ms - 208.10ms │                  slower │                         │                          slower │
│                              │                     │             868% - 916% │                -        │                         1% - 4% │
│                              │                     │     183.28ms - 187.37ms │                         │                 2.66ms - 8.43ms │
├──────────────────────────────┼─────────────────────┼─────────────────────────┼─────────────────────────┼─────────────────────────────────┤
│ this-change [update-reflect] │ 198.47ms - 202.64ms │                  slower │                  faster │                                 │
│                              │                     │             841% - 889% │                 1% - 4% │                        -        │
│                              │                     │     177.64ms - 181.91ms │         2.66ms - 8.43ms │                                 │
└──────────────────────────────┴─────────────────────┴─────────────────────────┴─────────────────────────┴─────────────────────────────────┘

My expectation would be that there would still be a benchmark name. On further investigation, this seems to be working as intended for small tables. However when partitioning a table into 3 small tables, the result becomes confusing due to a lack of benchmark name field.

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.

Option to collate result table by measurement (for multiple measurements)
7 participants