-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Disable DebugProbes.enableCreationStackTraces by default #4028
Conversation
* Adjust tests and the documentation * Rationale: this option implies significant and non-trivial overhead yet its utility is unclear (e.g. we got a multitude of signals that this option brings no use and none that it's useful even when asked explicitly) Fixes #3783
b9cea3c
to
23649ba
Compare
I've tested it locally -- IDEA debugger keeps working with this change. |
* This option can be useful during local debug sessions, but is recommended | ||
* to be disabled in production environments to avoid stack trace dumping overhead. | ||
* to be disabled in production environments to avoid performance overhead of keeping debug probes enabled. |
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.
I understood the previous version better. With enableCreationStackTraces
, you copy a bunch of stackframe descriptions with many strings around on every coroutine creation, which is expensive, ok, this makes sense. What does the new text mean? This option changes the behavior while the debug probes are enabled, it doesn't force them to be enabled longer than needed, right?
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.
This text is indeed misleading, changed it.
you copy a bunch of stackframe descriptions with many strings around on every coroutine creation
Surprisingly, this one is quite fast. The heaviest performance hitter is Throwable.fillInStackTrace
. JVM upcall + stack unwinding are quite costly.
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.
About exceptions performance: https://shipilev.net/blog/2014/exceptional-performance/
That's why we have overridden fillInStackTrace
in some places
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.
Here are some more changes that I think are in line with the spirit of the PR: https://gist.github.com/dkhalanskyjb/e04623b28fc274db1c52f5ea2d491ce7
Sure, mind pushing them? |
Fixes #3783