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

Benchmarks should also run against AOT targets #613

Closed
jakemac53 opened this issue Apr 18, 2022 · 0 comments · Fixed by #698
Closed

Benchmarks should also run against AOT targets #613

jakemac53 opened this issue Apr 18, 2022 · 0 comments · Fixed by #698
Assignees

Comments

@jakemac53
Copy link
Contributor

The existing benchmarks should be runnable, but possibly some helper scripts should be created, or just the README should be updated to mention how to run the benchmarks in that mode.

All flutter apps I believe ship in this mode, so if anything it is probably the most important mode to test in.

@osa1 osa1 self-assigned this Apr 20, 2022
@osa1 osa1 closed this as completed in #698 Jul 7, 2022
osa1 added a commit that referenced this issue Jul 7, 2022
This merges two benchmark packages into one to make it easier to run benchmarks
on Golem and locally.

`api_benchmark` is not merged: it runs a web server and benchmarks performance
in an actual browser. We should revisit those benchmarks to see if they make
sense and see if it's a good idea to merge them into the new benchmark package
in this PR.

Changes:

- Remove `protobuf_benchmarks`, `query_benchmark`. Move benchmarks in both to
  `benchmarks`.

- Use `benchmark_harness` in all benchmarks. Previously only
  `protobuf_benchmarks` used `benchmark_harness`.

- Because `benchmark_harness` reports 10x the actual number
  (https://github.com/dart-lang/benchmark_harness/issues/30), to be compatible
  with the old `query_benchmark` results (which are stored in Golem) and
  generally report accurate results, `BenchmarkBase` class overrides the
  relevant method(s) from `benchmark_harness.BenchmarkBase` to divide the
  result by 10 before reporting.

  Outputs before:

      RunTimeRaw(protobuf_decode): 973 us
      RunTimeRaw(protobuf_decode_json): 3914 us
      RunTimeRaw(protobuf_encode): 3135 us
      RunTimeRaw(protobuf_encode_json): 4606 us
      RunTimeRaw(protobuf_decode): 30 us

  Outputs after:

      protobuf_query_decode(RunTimeRaw): 973.4907766990291 us.
      protobuf_query_decode_json(RunTimeRaw): 3925.9745098039216 us.
      protobuf_query_encode(RunTimeRaw): 3115.623076923077 us.
      protobuf_query_encode_json(RunTimeRaw): 4568.952272727272 us.
      protobuf_query_set_nested_value(RunTimeRaw): 18.9181422625804 us.

  All benchmarks (query_benchmark and others) use this new class.

- Directory structure:

  - `datasets/` has benchmark inputs (i.e. binary encoded protos)

  - `protos/` has benchmark protos

  - `lib/` has code common to all benchmarks (file operations, benchmark class)

  - `tool/compile_protos.sh` compiles all protos

  - `tool/run_protoc_plugin.sh` runs protoc plugin from `../protoc_plugin`.
    Used by `compile_protos.sh`.

  - `tool/compile_benchmarks.dart` compiles benchmark programs to native and
    JS, in parallel

  - `bin/` contains benchmarks

  - `protoc_version` is used by Golem to download the right version of protoc

- Each benchmark now has its own executable. This is to allow benchmarking
  changes in isolation. For example, to avoid changing encoding benchmark
  results when decoding code was modified, because of global analyses or JIT.

- `protobuf_benchmarks/` inputs no longer use [`benchmarks.proto`][1]. Instead
  the input files are now the `payload` fields of `BenchmarkDataset`s, and
  input file name specifies message type.

Fixes #613

Golem cl 4855277

[1]: https://github.com/google/protobuf.dart/blob/15d9eed9f44dc3bab0738d33921d2608f3b158b5/protobuf_benchmarks/benchmarks.proto
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 a pull request may close this issue.

2 participants