Remove expensive debug assertion from dynlink

[lpw25]

Dynlink_common.check includes a call to an invariant function that just contains assertions. These assertions turn out to be expensive and don't really provide any benefit. This PR removes the call to invariant.

[dra27]

I can't decide if the intention of let _ = function_name is more or less clear than switching off warning 32!

Equally if the check isn't useful, why not remove it entirely or, if it's potentially useful at some point in the future, change L278 to be if debugging_dynlink then State.invariant state;?

[dra27]

Why not instead have let[@ocaml.warning "-32"] invariant t = above?

[gasche]

Personally I prefer let _ = inveriant or let () = ignore invariant, which is a clear idiom (to me) and less visually noisy.

[gasche]

The invariant was added in 6526a0c (#1063).

These assertions turn out to be expensive and don't really provide any benefit.

Can you provide a bit more details?

• What does "expensive" mean here? (The check is a bunch of call to String.Set.subset, do those sets get really large?)
• What does "no benefit" mean here? The commit that introduces this change (among others) is called "make (nat)dynlink sound", and that seems beneficial enough. But then looking briefly at the code, it may be that some (or all?) of those checks can be statically discharged by carefully reading the implementation of the check function.

[lpw25]

What does "expensive" mean here?

I think the sets get very large. We have some cmxs files being loaded, that takes 14s and perf says that 53% of the time is spent in the invariant function.

What does "no benefit" mean here?

The function is only assert statements. If the code is correct then they can never be triggered. I think this is just a case of someone accidentally leaving in some debugging code when they had finished writing the feature.

if the check isn't useful, why not remove it entirely

The function is good documentation of the invariants of the data. It is a common idiom for a data-structure to include a function called invariants of this form. These are often useful when writing tests, but they are useful anyway as documentation.

[xavierleroy]

I agree that these assertions should not be run by default. However, I don't like dead code left there just in case, with hacks to silence the "unused code" warning. In order of decreasing preference, I would rather

1. remove the code entirely
2. comment it out
3. write it as

let debug = false
...
  if debug then check_invariants()
...

[lpw25]

I went with option 1, rebased and added a Changes entry.

[xavierleroy]

Looks good to me! Thanks.