base icon indicating copy to clipboard operation
base copied to clipboard

Base sets Printexc.record_backtrace by default

Open jberdine opened this issue 6 years ago • 3 comments

The module initialization code of base calls Backtrace.initialize_module, which (unless OCAMLRUNPARAM is set) calls Printexc.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.

jberdine avatar Jun 30 '19 22:06 jberdine

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

yminsky avatar Jul 01 '19 00:07 yminsky

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, Map.find_exn could go either way depending on usage (either a missing key is used to represent the map sparsely, or it's a failure mode, the library can't know). Probably a good eventual situation is for libraries to use raise_notrace for their internal control flow, and expose external interfaces so that exceptions cross the boundary only in failure situations. But at the moment there is a lot of code that predates raise_notrace.

Also note that since 4.04, the overhead of recording backtraces seems to apply even on code compiled without -g: https://github.com/ocaml/ocaml/issues/8780.

jberdine avatar Jul 01 '19 10:07 jberdine

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.

yminsky avatar Jul 02 '19 15:07 yminsky