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

Summary table loses some of the extra information from go benchmarks when running with multiple CPU scenarios #242

Open
jaqx0r opened this issue May 4, 2024 · 1 comment

Comments

@jaqx0r
Copy link

jaqx0r commented May 4, 2024

https://github.com/google/mtail/actions/runs/8951254978?pr=858

The table includes lines like

BenchmarkStore/Add 818.6 ns/op 821.1 ns/op 1.00
BenchmarkStore/Add-1 778.3 ns/op 780.2 ns/op 1.00
BenchmarkStore/Add-1 779.9 ns/op 780.2 ns/op 1.00

which is confusng because these are three different scenarios -- the number of procs per the -cpu flag to go test -bench.

This is being recorded in the JSON file found in https://github.com/google/mtail/actions/runs/8951254978/job/24587370213?pr=858

   {
      name: 'BenchmarkStore/Add',
      value: 818.6,
      unit: 'ns/op',
      extra: '1260484 times\n1 procs'
    },
    {
      name: 'BenchmarkStore/Add-1',
      value: 778.3,
      unit: 'ns/op',
      extra: '1538625 times\n2 procs'
    },
    {
      name: 'BenchmarkStore/Add-1',
      value: 779.9,
      unit: 'ns/op',
      extra: '1574536 times\n4 procs'

in this extra field. It would be useful if the name column included the "N procs" part as well to differentiate the names. Or it would be fine to leave the name alone as BenchmarkStore/Add-1-4 and not extract the procs count into a special "extra"

@jaqx0r jaqx0r changed the title Summary output loses some of the extra information from go benchmarkes Summary table loses some of the extra information from go benchmarkes May 4, 2024
@jaqx0r jaqx0r changed the title Summary table loses some of the extra information from go benchmarkes Summary table loses some of the extra information from go benchmarks when running with multiple CPU scenarios May 4, 2024
@jaqx0r
Copy link
Author

jaqx0r commented May 4, 2024

There's a further bug here -- the name BenchmarkStore/Add loses the test name suffix. It looks like the benchmark-action is incorrectly assuming the suffix -1 is the number of procs, but go test -bench does not append a suffix for the default 1 CPU option.

So I don't think this attempt at parsing out N procs here is a useful function and should be removed. I'll offer a PR if you agree.

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

No branches or pull requests

1 participant