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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: enable async stack trace by default #6539
Conversation
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Codecov Report
@@ Coverage Diff @@
## main #6539 +/- ##
==========================================
+ Coverage 74.01% 74.06% +0.04%
==========================================
Files 983 989 +6
Lines 160712 161778 +1066
==========================================
+ Hits 118952 119820 +868
- Misses 41760 41958 +198
Flags with carried forward coverage won't be shown. Click here to find out more.
馃摚 We鈥檙e building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
/// With `debug_assertions` on, this span will be disabled statically to avoid affecting | ||
/// performance too much. Therefore, `verbose` mode in [`TraceConfig`] is ignored. |
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.
Previously I thought the bad performance with verbose
span, non-verbose
configuration, and debug profile was caused by the extra code of matching the thread-local context. But actually, I just forgot to pass the config to the trace root and always use verbose: true
configuration, as above. 馃ぃ
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.
LGTM! 馃帀
* fix config Signed-off-by: Bugen Zhao <i@bugenzhao.com> * turn on trace by default Signed-off-by: Bugen Zhao <i@bugenzhao.com> * try use default verbose in CI Signed-off-by: Bugen Zhao <i@bugenzhao.com> * cleanup comments Signed-off-by: Bugen Zhao <i@bugenzhao.com> Signed-off-by: Bugen Zhao <i@bugenzhao.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
fix: enable async stack trace by default (#6539) * fix config Signed-off-by: Bugen Zhao <i@bugenzhao.com> * turn on trace by default Signed-off-by: Bugen Zhao <i@bugenzhao.com> * try use default verbose in CI Signed-off-by: Bugen Zhao <i@bugenzhao.com> * cleanup comments Signed-off-by: Bugen Zhao <i@bugenzhao.com> Signed-off-by: Bugen Zhao <i@bugenzhao.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Signed-off-by: Bugen Zhao <i@bugenzhao.com> Co-authored-by: Bugen Zhao <i@bugenzhao.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
Previously the async stack trace is only enabled by default only if the user launches with
risedev
. This PR also enables it by default if no arg is specified, which is for production (cloud) deployment.I've benched the release build with async stack trace = (verbose, on, off) and hardly found any performance regression. So I guess we're okay to enable it in production.
I'd also like to cherry pick this fix to the release branch. 馃
Checklist
./risedev check
(or alias,./risedev c
)Refer to a related PR or issue link (optional)