cc @mshinwell, @xavierleroy

846ce83 (the change to runtime/backtrace.c to use caml_alloc_shr instead of caml_alloc, and get rid of a large on-stack buffer) looks like a very nice change on its own -- depending on how fast you think the present PR is going to get a decision, it may make sense to send it as a separate pre-PR.

@gasche @stedolan: Doesn't this risk to put a lot of short-lived values in the major heap? I can see this happening when get_raw_backtrace is used in conjunction with reraise_with_backtrace.

When I first thought about the change, my idea was that callstacks are typically (1) fairly large (often larger than Max_young_wosize?) and (2) likely to get stored later for auditing, and thus long-lived, or to come just before the program errors out, and thus we don't care.

(I had another paragraph here which I guess was confused about when backtraces are generated by the compiler when using exceptions for control-flow, and people are not using raise_notrace. I need to remind myself of how that actually works or let someone else comment on that part -- it could be indeed that this use-case generates a lot of backtraces and discards most of them immediately, but maybe not.)

The situation with exceptions are used for control flow, where backtraces are unnecessarily generated and then discarded, is indeed annoyingly common. The importance of performance in this situation is why caml_stash_backtrace still has a static global buffer.

This change affects only Printexc.get_raw_backtrace (and a few other functions in Printexc), which use a seperate codepath from caml_stash_backtrace. I don't think this is used anywhere performance-sensitive.

Will the backtraces collected by statmemprof be affected? (Those are expected to follow the lifetime of the memory block they track, so we can expect that many of them will be short-lived.)

No, statmemprof does not use this mechanism to collect callstacks. Statmemprof is interested in the current callstack, so it walks the stack to collect it. These functions concern the backtrace of the most recent exception, which is stored in a global buffer.

I changed this patch to remove the sharing of sigaltstacks. The reasoning why this was OK was based on considering only SIGSEGV; there are other signals and they share a stack.

This change affects only Printexc.get_raw_backtrace (and a few other functions in Printexc), which use a seperate codepath from caml_stash_backtrace.

This is true.

I don't think this is used anywhere performance-sensitive.

This is perhaps less true these days. For example, Fun.protect calls Printexc.get_raw_backtrace if an exception occurs. I would still hope that Fun.protect is not used in conjunction with functions that raise often, but who knows...

I looked for usage of Misc.try_finally (a parent of Fun.protect)
in the compiler codebase, I have the impression that all
use-cases concern blocks where the exception path is indeed
exceptional. It is also used at a fairly coarse
granularity (typically, the cleanup action is to remove
a temporary file), so I think additional costs would be
amortized.

I haven't seen a use of Fun.protect around exceptions used for
control-flow, but that seems entirely possible to me. (For example,
a SAT solver where the "restart" action, every N decision steps,
is implemented by raising an exception, may use Fun.protect to unwind
the decision stack.) I think it would make sense to also offer a
Fun.protect_notrace function, cousin of raise_notrace, that does
not capture the call stack and uses a reraise_notrace.

Let me know if you like the idea and I'll shoot a separate PR or issue report.

I think it would make sense to also offer a
Fun.protect_notrace function, cousin of raise_notrace, that does
not capture the call stack and uses a reraise_notrace.

Currently (both before and after this patch), if raise_notrace is used, then Printexc.get_raw_backtrace is very fast (it does not allocate at all, since there is no backtrace). So I don't think this function is necessary. Effectively, Fun.protect does a reraise_notrace if the original exception came from raise_notrace, which I think is the right behaviour.

Ah, indeed. Sorry for the internal confusedness.

I looked for usage of Misc.try_finally (a parent of Fun.protect)
in the compiler codebase, I have the impression that all
use-cases concern blocks where the exception path is indeed
exceptional. It is also used at a fairly coarse
granularity (typically, the cleanup action is to remove
a temporary file), so I think additional costs would be
amortized.

In the following review of uses of try_finally in the compiler: #2118 (comment), I have found several cases where it is used for expressing temporary mutation, where the finally just rolls back some state. This is an idiomatic use of unwind-protect, and is a use-case that can occur in algorithmically-intensive situations (e.g. if exceptions are used for control flow). It seems incorrect to me to assume in general that resources are few and disposal is expensive (amortizes the cost of recording the stack trace).

I haven't seen a use of Fun.protect around exceptions used for
control-flow, but that seems entirely possible to me. (For example,
a SAT solver where the "restart" action, every N decision steps,
is implemented by raising an exception, may use Fun.protect to unwind
the decision stack.)

Yes, this is a valid use-case, actually I have found 5 of them in the aforementioned audit. Control flow is the intended usage of Fun.protect, when distinguished from bracket-style resource management that would do masking for async exceptions. This use-case was in fact the reason put forward in favour of including Fun.protect with its current type, whatever the fate of asynchronous exceptions would be.

Out of curiosity, how expensive is it to call Printexc.get_raw_backtrace immediately after Printexc.raise_with_backtrace, for instance in the case where the user has nested Fun.protects?

This discussion got side-tracked by some questions about whether allocating backtraces on the major heap would have a noticeable performance impact.

I've just rebased this patch to trunk. On trunk, since #8691, plain caml_alloc should not trigger callbacks. So, the new version of this patch just calls caml_alloc (which allocates on the minor or major heap as appropriate).

However, when rebasing this I noticed a bug in #8691, in that it does not always prevent caml_alloc from triggering callbacks. This bug is fixed in #8897, so we shouldn't merge this patch until after #8897.

#8897 is merged, and this branch has just passed precheck (300), so I think this is ready to merge.

The second commit about removing the stack-allocated buffer looks good to me. (I would happily help with a review of the first commit but the technical details still fly over my head.)

I'd quite like to get this into 4.10. Are there any remaining issues?

Thanks!

I hope it's the right place to report it: I have a bunch of errors like the following one when I do make depend inside the runtime directory:

gcc -MM -D_FILE_OFFSET_BITS=64 -D_REENTRANT -DCAML_NAME_SPACE   *.c | \
  sed -e 's/\([^.]*\)\.o/\1_b.$(O)/' > .depend
signals_nat.c:192:2: error: #error "CONTEXT_SP is required if HAS_STACK_OVERFLOW_DETECTION is defined"
 #error "CONTEXT_SP is required if HAS_STACK_OVERFLOW_DETECTION is defined"
  ^~~~~