-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Benchmark movements: |
I suppose that this regression is only in benchmarking, and does not affect the runtime of the binary code, but:
|
80a2826
to
609280d
Compare
Benchmark movements: full_committer_flow performance regressed! |
There was a problem hiding this 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
Previously, dorimedini-starkware wrote…
OK, I'll look into it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)