Skip to content

Conversation

@osa1
Copy link
Member

@osa1 osa1 commented Jun 28, 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
    (Why is the benchmark reporting 10-times higher values? dart-lang/tools#791), 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. Instead
    the input files are now the payload fields of BenchmarkDatasets, and
    input file name specifies message type.

Fixes #613

Golem cl 4855277


cc @mkustermann

@osa1 osa1 requested a review from sigurdm June 28, 2022 10:00
Copy link
Collaborator

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

LGTM

@mkustermann
Copy link
Collaborator

TODO: Do we need to tweak number of iterations in query_benchmark
benchmarks?

👍 Would be good numbers stay compatible so we keep old history.

@mkustermann
Copy link
Collaborator

mkustermann commented Jun 28, 2022

Another thing you may want to consider: Check the pubspec.lock file (generated by pub get from pubspec.yaml) into the repository (only for the benchmark folder). This will ensure that if a new minor version of a dependency (e.g. package:fixnum) is published, we won't get performance result changes at random commits - instead upgrades of 3rd party packages will be visible by the commit that updates the lock file.

@osa1
Copy link
Member Author

osa1 commented Jun 30, 2022

I'm updating Golem to use benchmarks added in this PR. Once that works I'll merge this PR.

Copy link

@sortie sortie left a comment

Choose a reason for hiding this comment

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

A couple thoughts :)

///
/// Overrides the relevant [bh.BenchmarkBase] method(s) to report accurate
/// results, rather than 10x the actual results.
abstract class BenchmarkBase extends bh.BenchmarkBase {
Copy link

Choose a reason for hiding this comment

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

Consider another name for this class to avoid confusion between the upstream BenchmarkBase and this modified one.

I am planning on fixing the 10x bug in BenchmarkBase once I have time to think up a migration scheme

@osa1 osa1 merged commit 7084682 into google:master Jul 7, 2022
@osa1 osa1 deleted the refactor_benchmarks branch July 7, 2022 11:14
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.

Benchmarks should also run against AOT targets

4 participants