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

chore: add profile in toml for flamegraph #304

Closed
wants to merge 1 commit into from

Conversation

aner-starkware
Copy link
Contributor

@aner-starkware aner-starkware commented Jul 14, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.74%. Comparing base (a8ac86e) to head (609280d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #304   +/-   ##
=======================================
  Coverage   70.74%   70.74%           
=======================================
  Files          38       38           
  Lines        2068     2068           
  Branches     2068     2068           
=======================================
  Hits         1463     1463           
  Misses        535      535           
  Partials       70       70           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [37.877 ms 38.889 ms 39.924 ms]
change: [+9.4500% +13.050% +16.440%] (p = 0.00 < 0.05)
Performance has regressed.

@aner-starkware
Copy link
Contributor Author

I suppose that this regression is only in benchmarking, and does not affect the runtime of the binary code, but:

  1. Perhaps this is also not good, as we want to benchmark the real runtime
  2. I don't know how to make sure that the actual runtime isn't affected by this change

Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [36.268 ms 37.150 ms 38.075 ms]
change: [+3.5793% +6.7633% +9.9058%] (p = 0.00 < 0.05)
Performance has regressed.

full_committer_flow performance regressed!
full_committer_flow time: [29.027 ms 29.209 ms 29.496 ms]
change: [+1.1842% +1.8211% +2.8506%] (p = 0.00 < 0.05)
Performance has regressed.
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severe

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @TzahiTaub)


Cargo.toml line 51 at r1 (raw file):

[profile.bench]
debug = true

this means benchmarking times will be slower, right?
consider a separate CI job that compiles without the --release flag

Code quote:

[profile.bench]
debug = true

@aner-starkware
Copy link
Contributor Author

Cargo.toml line 51 at r1 (raw file):

Previously, dorimedini-starkware wrote…

this means benchmarking times will be slower, right?
consider a separate CI job that compiles without the --release flag

OK, I'll look into it

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)


Cargo.toml line 51 at r1 (raw file):

Previously, aner-starkware wrote…

OK, I'll look into it

for a temporary solution it's fine now,
and one could argue we only care about the relative diff in a benchmark, not the flat time, so this may be unneeded... unblocking

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

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.

3 participants