Running the testsuite with the current trunk:

real	14m8.349s
user	20m52.485s
sys	0m46.666s

Event count (approx.): 5024892760014
Overhead  Command          Shared Object                       Symbol
  16.43%  Domain0          ocamlrun                            [.] caml_check_for_pending_signals
  14.16%  Domain0          ocamlrun                            [.] caml_interprete
   6.67%  Domain1          ocamlrun                            [.] caml_check_for_pending_signals
   5.12%  Domain1          ocamlrun                            [.] caml_interprete
   4.72%  Domain3          ocamlrun                            [.] caml_check_for_pending_signals
   4.60%  Domain2          ocamlrun                            [.] caml_check_for_pending_signals
   3.74%  Domain3          ocamlrun                            [.] caml_interprete
   3.67%  Domain2          ocamlrun                            [.] caml_interprete
   2.29%  Domain4          ocamlrun                            [.] caml_check_for_pending_signals
   1.34%  Domain4          ocamlrun                            [.] caml_interprete
   1.15%  Domain0          ocamlrun                            [.] caml_ensure_stack_capacity
   0.96%  Domain0          ocamlrun                            [.] caml_MD5Transform
   0.83%  Domain3          ocamlrun                            [.] lf_skiplist_lookup
   0.82%  Domain2          ocamlrun                            [.] caml_sample_gc_stats
   0.81%  Domain3          ocamlrun                            [.] caml_sample_gc_stats
   0.78%  Domain1          ocamlrun                            [.] caml_sample_gc_stats
   0.73%  Domain1          ocamlrun                            [.] caml_ensure_stack_capacity
   0.55%  Domain3          ocamlrun                            [.] caml_ensure_stack_capacity
   0.54%  Domain3          ocamlrun                            [.] caml_find_code_fragment_by_pc
   0.54%  Domain2          ocamlrun                            [.] caml_ensure_stack_capacity

Note that we spend as much time checking for signals as interpreting bytecode instructions!

Running the testsuite with this PR:

real	8m32.072s
user	10m16.386s
sys	0m45.736s

Event count (approx.): 2483613262503
Overhead  Command          Shared Object                       Symbol
  22.67%  Domain0          ocamlrun                            [.] caml_interprete
   4.21%  Domain1          ocamlrun                            [.] caml_interprete
   3.20%  Domain3          ocamlrun                            [.] caml_interprete
   3.11%  Domain2          ocamlrun                            [.] caml_interprete
   2.36%  Domain0          ocamlrun                            [.] caml_ensure_stack_capacity
   1.96%  Domain0          ocamlrun                            [.] caml_MD5Transform
   1.66%  Domain4          ocamlrun                            [.] caml_interprete
   0.86%  Domain0          ocamlrun                            [.] caml_shared_try_alloc
   0.83%  Domain1          ocamlrun                            [.] caml_ensure_stack_capacity
   0.80%  Domain0          ocamlrun                            [.] caml_thread_code
   0.74%  Domain0          libm-2.31.so                        [.] __sin_fma
   0.68%  Domain3          ocamlrun                            [.] caml_ensure_stack_capacity
   0.68%  Domain0          ocamlrun                            [.] mark
   0.66%  Domain2          ocamlrun                            [.] caml_ensure_stack_capacity
   0.64%  Domain3          ocamlrun                            [.] lf_skiplist_lookup
   0.58%  Domain2          ocamlrun                            [.] caml_sample_gc_stats
   0.57%  Domain1          ocamlrun                            [.] caml_sample_gc_stats
   0.55%  Domain3          ocamlrun                            [.] caml_sample_gc_stats
   0.53%  Domain0          [unknown]                           [k] 0xffffffffb8275ee7
   0.52%  Domain0          ocamlrun                            [.] pool_sweep
   0.52%  ld               libbfd-2.34-system.so               [.] bfd_link_hash_traverse

Do we need sequential consistency on the load in caml_check_for_pending_signals? Relaxed may be sufficient,

Yes, this sounds very plausible to me, and it matches what you did in lf_skiplist.c . Maybe an expert like @maranget can confirm. In the meantime I'll update this PR to use relaxed loads.

Looking at the generated assembly code, I realized that Linux has NSIG = 65, so we actually use two words instead of one, as I was hoping. Yet the code seems pretty efficient.

@xavierleroy may I ask what command you had used to generate these results? It seems quite useful and important.

This is good old perf under Linux. Nothing fancy!

    cd testsuite
    make clean
    perf record make all
    perf report

Looking at the generated assembly code, I realized that Linux has NSIG = 65, so we actually use two words instead of one, as I was hoping. Yet the code seems pretty efficient.

Depending on how keen we are to keep it in one word on Linux, we've got some wasted space at 0..

Depending on how keen we are to keep it in one word on Linux, we've got some wasted space at 0..

Yes, I was thinking along these lines! POSIX 2008 says that signal numbers are positive integers, and that "The value 0 is reserved for use as the null signal (see kill())", so we can fit signals 1 to 64 into one word... Stay tuned!

OK, I just pushed a version where signal numbers start at 1, not at 0. The compiled C code is even nicer than before :-)

I noticed other opportunities for simplifications in signal handling, but this will be for another PR.

Updated code looks correct.

One last thing we could do (which came out of a conversation with @dra27) is use ctz for the index to start checking from. Might be an over-optimisation though.

I did consider clz / find first bit set (it's a canonical example of use of these instructions!) but they are not portable, i.e. we'd have a builtin function for GCC / Clang and a library function or hand-written code for MSVC.

I did consider clz / find first bit set (it's a canonical example of use of these instructions!) but they are not portable, i.e. we'd have a builtin function for GCC / Clang and a library function or hand-written code for MSVC.

MSVC has _BitScanReverse.

For context, polling in bytecode is not supposed to check all signals one by one. The performance impact is more a symptom of the regressions I commented about in my review of the multicore PR (#10831 (comment)). In fact, why should comparing the young_pointer to the young_limit in interp.c not be enough, as done for polling in native?

However anything that rids us of the signals_are_pending cache variable from old trunk is good to take; this is beneficial for caml_enter_blocking_section.

In fact, why should comparing the young_pointer to the young_limit in interp.c not be enough, as done for polling in native?

That's an excellent question. I can think of historical reasons (the bytecode interpreter came first, relying on a global "something_to_do" flag that was set by signal handlers and some other runtime functions too), but nothing more compelling.

I also don't see the need for caml_check_for_pending_signals() in interp.c for POPTRAP & CHECK_SIGNALS.

de70ae5 introduced these caml_check_for_pending_signals() calls as part of ocaml-multicore#722 which fixed up the signals_alloc testcase. Notice that caml_check_for_pending_signals was also added to the macro Enter_gc in this change.

@Engil might have insight into why it was needed.

For the POPTRAP and CHECK_SIGNALS addition, I added these following some observations made comparing the bytecode and native versions on the signals_alloc testcase.

The gist of it was, at the time, that without these polling point, the testcase would fail here:
https://github.com/ocaml/ocaml/blob/trunk/testsuite/tests/callback/signals_alloc.ml#L17
It would not print12345, but rather 12435. After debugging for a while I suspected these polling point were necessary to get the behavior the testsuite expected, but maybe the test assertions are a bit too strict?

Nobody dares to accept this PR, but I think it's in good shape now, so I'll merge tomorrow if there is no more objections.

I'll have a look at the signals_alloc test. Perhaps it's over-constrained and should not prevent a nicer, more efficient implementation of polling in the bytecode interpreter. If so we'll do that in a separate PR.

(I tried to give an approving review but it's disabled at the moment.)

Nobody dares to accept this PR, but I think it's in good shape now, so I'll merge tomorrow if there is no more objections.

I think so too. Do not hesitate to request reviews when appropriate.

Right, there were temporary restrictions in place on who can approve. Should be back to normal now.