-
Notifications
You must be signed in to change notification settings - Fork 129
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
Base sets Printexc.record_backtrace by default #77
Comments
Seems like a reasonable point. I feel pretty strongly that recording backtraces is the right default, but I agree it is odd to have this enforced at the library level, and surprising for users of Base. FWIW, I believe it's only a performance issue when you're using exceptions a lot, which I think would mostly arise when you're using exceptions for fine-grained control flow. This is not a good pattern most of the time, and when you do need it, you can use Exn.raise_without_backtrace |
Yes, this is only going to hurt when exceptions are used a lot, and especially when they are raised from deeply-recursive functions. I'm not sure that control-flow versus handlable-crash is an easy distinction to make for libraries though. For instance, Also note that since 4.04, the overhead of recording backtraces seems to apply even on code compiled without |
This doesn't line up with my experience. Code that uses exceptions for control flow typically is doing so explicitly, rather than relying on exceptions thrown by Map.find_exn. There are tradeoffs, but I think a pretty good compromise is having slower-but-more-informative exceptions by default, with the ability to raise uninformative exceptions when needed. That said, having this choice forced upon you by Base isn't great. |
The module initialization code of base calls
Backtrace.initialize_module
, which (unlessOCAMLRUNPARAM
is set) callsPrintexc.record_backtrace true
.This is a significant performance foot-gun, and as far as I can see, not documented anywhere outside the code.
Aside: It's not clear to me that libraries have any business setting global state like this, but presumably there was an argument for this being the right default.
The text was updated successfully, but these errors were encountered: