Weak.get_copy: copying continuations is unsafe, so don't copy them.

[yallop]

It's not generally safe to copy continuations. However, Weak.get_copy shallow-duplicates continuations, which can lead to program crashes. Here's an example program:

type _ Effect.t += A : unit Effect.t | B : string Effect.t

let () =
  let w :  (unit, unit) continuation Weak.t = Weak.create 1 in
  let ka : (unit, unit) continuation option ref = ref None in

  begin try Effect.perform A; print_endline (Effect.perform B) with
  | effect A, k ->
     Weak.set w 0 (Some k);
     ka := Some k
  | effect B, _ -> ()
  end;

  let wc = Weak.get_copy w 0 in
  Effect.Deep.continue (Option.get !ka) ();
  Effect.Deep.continue (Option.get wc) ()

Running the program produces a segmentation fault.

The problem (identified with Claude's help): duplicating the continuation produces two continuations that share the same fiber, and the mechanism intended to prevent re-execution (setting the fiber to null after execution) only affects one of them. In the example program, the first call to continue returns to perform A, and the program then advances to perform B, then the second call to continue passes a unit value back to the perform B call, which expects a string.

The fix is to treat continuations like custom values in Weak.get_copy, and not duplicate them.

[smuenzel]

Copying continuations definitely elides the checks that are built into caml_continuation_use, so preventing copies is a good fix.

[dra27]

Seems worth putting the repro in the testsuite?

[stedolan]

This is definitely an improvement and we should merge it.

However, since there are many other problems with Weak.get_copy, maybe it's time to remove it? (For compatibility, "remove" would probably mean "make it an alias to Weak.get and deprecate")

[jberdine]

However, since there are many other problems with Weak.get_copy, maybe it's time to remove it?

FWIW I would be in favor of removing it. I have been bitten a few times by get_copy being "broken" in all but the simplest cases. To be fair though, most of those were due to the uses of get_copy internal to Weak that were removed by #12131. On the other hand, if even the hot uses in Weak use full get instead of get_copy, the performance argument for get_copy is hard to make as far as I understand.

[stedolan]

My understanding is that the main performance argument for get_copy is that the old, old implementation of weak hashtables didn't store the hashes, so that it called get_copy on every probed bucket, not just on ones with the matching hash. Using get instead of get_copy here would have kept many values alive unnecessarily, just because they were stored near a live value in the hashtable.

This was inefficient, and was fixed 18 years ago, along with other perf bugs in that era's weak.ml.

Unfortunately, two years prior to that a hashconsing library was published that became reasonably widely used and got copied into various projects. That library copypasted the old, slow weak hashtable implementation from 2006-era weak.ml, and it would not surprise me if its performance in some programs really did depend on get_copy.

The current release of that library has switched to using Weak.get and no longer uses get_copy at all. All of the uses I've seen of Weak.get_copy in the wild are copies of old versions of this library, so I think we could just deprecate get_copy and tell anyone who notices to upgrade to the latest version of ocaml-hashcons.