Fun.protect has some overhead in contexts combining temporary mutations and exceptions raised for control flow (unless those are raised with raise_notrace). Have you looked at whether it might have been used with expectations of efficiency in some places? (I tried to, but it was not clear to me that it was not the case.)

Other than that, looks good to me.

This is a good question, thanks. Most uses of protect_refs in the compiler codebase are for interactive programs doing IO inside, so they're not performance-sensitive in this way. But there are a couple use-cases in the type-checker:

1. an interesting one is the without_warning helper that runs some typechecking code with warnings disabled, and
2. the most interesting one is the set_mode_pattern helper in typing/ctype.ml, which is used at a fine-grained granularity in the Ctype.unify3 function. If one use-case is performance-sensitive, it's this one. Exceptions do occur there on a not-completely-exceptional basis (unification errors), but they're not "used for control-flow" in the sense of backtracking for example.

My gut feeling is that there will not be any noticeable performance regression, and that if we found any, the solution would be to use raise_notrace in the relevant part of the codebase (not to rollback the present change). But it would be better to test this gut feeling, and I'm not sure of how I want to do that. (cc @trefis, who has done some type-checking performance measurement.) The simplest idea is to take a hard-to-typecheck file, typically camlinternalFormat.ml, and verify that there is no noticeable type-checking-time degradation.

Thanks, the discussion is interesting, because it is a good occasion to confront the implementation of the new Fun.protect to real code.

1. an interesting one is the without_warning helper that runs some typechecking code with warnings disabled, and

Warning.disabled is a switch, so the good practice to avoid overhead of nested Fun.protects is to call it conditionally:

let without_warnings f =
  if !disabled then f ()
  else Misc.protect_refs [Misc.R(disabled, true)] f

but here really it does not matter (see below).

2. the most interesting one is the set_mode_pattern helper in typing/ctype.ml, which is used at a fine-grained granularity in the Ctype.unify3 function. If one use-case is performance-sensitive, it's this one. Exceptions do occur there on a not-completely-exceptional basis (unification errors), but they're not "used for control-flow" in the sense of backtracking for example.

So it is a priori unclear that this is the pathological case we are looking for, say with nested calls to Fun.protect in a success path which repeatedly end with an exception raised and caught.

My gut feeling is that there will not be any noticeable performance regression, and that if we found any, the solution would be to use raise_notrace in the relevant part of the codebase (not to rollback the present change). But it would be better to test this gut feeling, and I'm not sure of how I want to do that. (cc @trefis, who has done some type-checking performance measurement.) The simplest idea is to take a hard-to-typecheck file, typically camlinternalFormat.ml, and verify that there is no noticeable type-checking-time degradation.

I did make world with the following patch applied:

diff --git a/utils/misc.ml b/utils/misc.ml
index f42b79350..7e900a5f2 100644
--- a/utils/misc.ml
+++ b/utils/misc.ml
@@ -51,6 +51,19 @@ let try_finally ?(always=(fun () -> ())) ?(exceptionally=(fun () -> ())) work =
 
 type ref_and_value = R : 'a ref * 'a -> ref_and_value
 
+let count = ref 0
+
+let argv () =
+  let s = ref "" in
+  Array.iter (fun a -> s := !s ^ a ^ " ") Sys.argv;
+  !s
+
+let _ = at_exit (fun () ->
+    let f = open_out_gen [Open_append] 0 "/tmp/ocaml" in
+    Printf.fprintf f "%s\n backtraces copied %n times\n" (argv()) !count;
+    close_out f
+  )
+
 let protect_refs =
   let set_refs l = List.iter (fun (R (r, v)) -> r := v) l in
   fun refs f ->
@@ -58,7 +71,7 @@ let protect_refs =
     set_refs refs;
     match f () with
     | x           -> set_refs backup; x
-    | exception e -> set_refs backup; raise e
+    | exception e -> incr count; set_refs backup; raise e
 
 (* List functions *)

The result is the following: https://pad.inria.fr/p/np_XYK5b2EmjLyfbuNE
Summary: zero backtraces are copied, except in exactly one file, camlinternalFormat.ml where they are copied 113 times. (The same exercise with make world.opt returns additional 14 backtrace copies with middle_end/flambda/flambda.ml.)

My impression is: 1) good call about camlinternalFormat, 2) this is negligeable performance-wise, 3) looking further, one finds that all of the 113 copies are caused by the type-checking of one function, fmtty_rel_det. This is probably undesirable algorithm-wise, so the lesson is that the adverse algorithmic effect is real, even if not noticeable (it could be much worse in situations that you allude to where Fun.protect would be used in combination with backtracking).

This is going to be food for thought when thinking about resource-management, but this patch is good to go (no need for additional benchmarks).

Thanks for the analysis! This is nice work.