-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Performance optimization: only compute stack traces when needed #16616
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
Conversation
test performance please |
performance test scheduled: 1 job(s) in queue, 0 running. |
test performance please |
performance test scheduled: 1 job(s) in queue, 1 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/16616/ to see the changes. Benchmarks is based on merging with main (4a11252) |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/16616/ to see the changes. Benchmarks is based on merging with main (4a11252) |
1 similar comment
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/16616/ to see the changes. Benchmarks is based on merging with main (4a11252) |
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.
#16566 addresses the problem in a different way, by making Diagnostic
not extend Exception
anymore. I think that is cleaner, overall. There's no good reason why Diagnostic
should be an exception. So I'd leave aside for now all changes to Diagnostic
in order not to cause conflicts with that PR. The changes to TypeError
LGTM.
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.
#16566 addresses this problem in a different way, by not making Diagnostic extend Exception. I think that's cleaner overall. So I'd leave out changes to Diagnostic in order not to cause conflicts with #16566. The changes to TypeError
LGTM. The changes to SuspendException
seem unnecessary but won't hurt.
TypeError extends Exception, but the stack trace is only needed for debugging purposes, either when using -Ydebug or when investigating cyclic errors.
I noticed that
Throwable#fillInStackTrace
(which is called from the constructor of Throwable) showed up while profiling the compiler. This is because several data structures we use (Diagnostic
andTypeError
) extendException
. Since the stack trace is only used for debugging in some rare cases and is expensive to compute, it's worth avoiding it whenever possible. This can be done either by passingfalse
to thewritableStackTrace
constructor parameter, by overridingThrowable#fillInStackTrace
or by extendingscala.util.control.NoStackTrace
.I ended up not touching
Diagnostic
in this PR because it will be changed in #16566 to not extendException
.