Re: igc, macOS avoiding signals

[Helmut Eller <eller.helmut@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3490 bytes --]

On Tue, Dec 31 2024, Eli Zaretskii wrote:
> We'd need to add a new function to process_pending_signals, which
> would process SIGPROF and maybe also SIGALRM.  The signal handlers for
> those would then only set a flag (not pending_signals, some other
> flag).

I implemented this with the two attached patches.  The trouble is that,
the recorded backtraces are not same.  This can be seen by looking at
the call tree produced by profiler.el and the attached profiler-test.el.
When add_sample is called in the signal handler, then the call tree for
the foo example looks so:

    ...
    1986 100%         main
    1986 100%           record-samples
    1986 100%             foo
    1074  54%               float-time
       0   0%   ...

When add_sample is called from process_pending_signals, it looks like
this:

    ...
    1986 100%         main
    1986 100%           record-samples
    1986 100%             foo
       0   0%   ...

Not the absence of float-time.  The reason for this is, that in
bytecode.c, maybe_quit is called before the function is pushed to the
backtrace with record_in_backtrace.  In the second patch, I moved this
call forward to before the function is popped with lisp_eval_depth--.
With this patch, the call tree includes float-time again:

    ...
    1989 100%         main
    1989 100%           record-samples
    1989 100%             foo
    1981  99%               float-time
       0   0%   ...

However, float-time has now 99% as opposed to 54% in the first call
tree.

A more complex pair of call trees is attached in the files
bar-0.report and bar-2.report.  A significant difference there is
in this section:

     ...
     781  73%                     animate-place-char
      19   1%                       delete-char
      16   1%                       floor
       4   0%                       undo-auto--undoable-change
       4   0%                         undo-auto--boundary-ensure-timer
      96   9%                       insert-char
      14   1%                         undo-auto--undoable-change
       6   0%                           undo-auto--boundary-ensure-timer
       5   0%                       beginning-of-line
     232  21%                       move-to-column
     ...

compared to the version with both patches applied:

     ...
     693  72%                     animate-place-char
      32   3%                       delete-char
      29   3%                       window-start
      43   4%                       insert-char
     309  32%                       move-to-column
     222  23%                       beginning-of-line
       8   0%                       undo-auto--undoable-change
       8   0%                         undo-auto--boundary-ensure-timer
       8   0%                           run-at-time
       8   0%                             timer-set-function
       8   0%                               timerp
       8   0%                                 vectorp
     ...
     
E.g. the percentage attributed to beginning-of-line is quite different
in those two versions (23% and 0%).   

I'm not sure if those differences are acceptable.  I also have no good
idea how to reduce it, except inserting more calls to maybe_quit.

(In eval_sub and Ffuncall, it would also help the profiler to move the
maybe_quit call forward before lisp_eval_depth--. This would only matter
for interpreted functions, not in byte compiled code.  Curiously,
apply_lambda doesn't call maybe_quit at all.)

Helmut



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Delay-processing-of-SIGPROF-to-the-next-safepoint.patch --]
[-- Type: text/x-diff, Size: 2359 bytes --]

From ed26a2e8fc92a321f0afeb38c1f88db46ec957a6 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Fri, 3 Jan 2025 17:27:10 +0100
Subject: [PATCH 1/2] Delay processing of SIGPROF to the next safepoint

* src/lisp.h (process_pending_profiler_signals): New function.
* src/profiler.c (pending_profiler_signals): New variable.
(handle_profiler_signal): Instead of calling add_sample,
set pending_signals and increment pending_profiler_signals.
(process_pending_profiler_signals): New function.
* src/keyboard.c (process_pending_signals): Call
process_pending_profiler_signals.
---
 src/keyboard.c |  1 +
 src/lisp.h     |  1 +
 src/profiler.c | 17 ++++++++++++++++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index e875e98fde6..5d6cebdc990 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -8191,6 +8191,7 @@ process_pending_signals (void)
   handle_async_input ();
   do_pending_atimers ();
   do_async_work ();
+  process_pending_profiler_signals ();
 }
 
 /* Undo any number of BLOCK_INPUT calls down to level LEVEL,
diff --git a/src/lisp.h b/src/lisp.h
index 48585c2d8a1..774667a4f9c 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -5938,6 +5938,7 @@ maybe_disable_address_randomization (int argc, char **argv)
 extern void malloc_probe (size_t);
 extern void syms_of_profiler (void);
 extern void mark_profiler (void);
+extern void process_pending_profiler_signals (void);
 
 
 #ifdef DOS_NT
diff --git a/src/profiler.c b/src/profiler.c
index 3db7fe0eb3e..47367982cab 100644
--- a/src/profiler.c
+++ b/src/profiler.c
@@ -387,6 +387,9 @@ add_sample (struct profiler_log *plog, EMACS_INT count)
 /* Hash-table log of CPU profiler.  */
 static struct profiler_log cpu;
 
+/* Number of unprocessed profiler signals. */
+static uintptr_t pending_profiler_signals;
+
 /* The current sampling interval in nanoseconds.  */
 static EMACS_INT current_sampling_interval;
 
@@ -402,7 +405,19 @@ handle_profiler_signal (int signal)
       count += overruns;
     }
 #endif
-  add_sample (&cpu, count);
+  pending_signals = true;
+  pending_profiler_signals += count;
+}
+
+void
+process_pending_profiler_signals (void)
+{
+  uintptr_t count = pending_profiler_signals;
+  if (count)
+    {
+      pending_profiler_signals = 0;
+      add_sample (&cpu, count);
+    }
 }
 
 static void
-- 
2.39.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Call-maybe_quit-at-a-different-point-to-the-help-the.patch --]
[-- Type: text/x-diff, Size: 1154 bytes --]

From ec3227c060f12ca137b5a5bd1e607b922a6dafec Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Fri, 3 Jan 2025 18:12:41 +0100
Subject: [PATCH 2/2] Call maybe_quit at a different point to the help the
 profiler.

* src/bytecode.c (exec_byte_code): In the docall sequence, move
the to maybe_quit forward immediately before lisp_eval_depth--.
This helps the profiler to see the function that was
interrupted by the SIGPROF signal.
---
 src/bytecode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bytecode.c b/src/bytecode.c
index 31f7404cbd1..53c200e2c18 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -784,7 +784,6 @@ #define DEFINE(name, value) [name] = &&insn_ ## name,
 		  }
 	      }
 #endif
-	    maybe_quit ();
 
 	    if (++lisp_eval_depth > max_lisp_eval_depth)
 	      {
@@ -829,6 +828,7 @@ #define DEFINE(name, value) [name] = &&insn_ ## name,
 	    else
 	      val = funcall_general (original_fun, call_nargs, call_args);
 
+	    maybe_quit ();
 	    lisp_eval_depth--;
 	    if (backtrace_debug_on_exit (specpdl_ptr - 1))
 	      val = call_debugger (list2 (Qexit, val));
-- 
2.39.5


[-- Attachment #4: profiler-test.el --]
[-- Type: application/emacs-lisp, Size: 1730 bytes --]

[-- Attachment #5: bar-0.report --]
[-- Type: text/plain, Size: 1532 bytes --]

       0   0% nil
    1056 100%   normal-top-level
    1056 100%     command-line
    1056 100%       command-line-1
    1056 100%         main
    1056 100%           record-samples
    1056 100%             bar
    1056 100%               animate-birthday-present
    1046  99%                 animate-string
      61   5%                   sit-for
      61   5%                     sleep-for
     805  76%                   animate-step
     781  73%                     animate-place-char
      19   1%                       delete-char
      16   1%                       floor
       4   0%                       undo-auto--undoable-change
       4   0%                         undo-auto--boundary-ensure-timer
      96   9%                       insert-char
      14   1%                         undo-auto--undoable-change
       6   0%                           undo-auto--boundary-ensure-timer
       5   0%                       beginning-of-line
     232  21%                       move-to-column
     132  12%                   primitive-undo
       6   0%                     undo-auto--undoable-change
      10   0%                 capitalize
      10   0%                   load-with-code-conversion
       8   0%                     hack-read-symbol-shorthands
       8   0%                       hack-local-variables--find-variables
       8   0%                         search-backward
       2   0%                     generate-new-buffer
       2   0%                       get-buffer-create
       0   0%   ...

[-- Attachment #6: bar-2.report --]
[-- Type: text/plain, Size: 1526 bytes --]

       0   0% nil
     959 100%   normal-top-level
     959 100%     command-line
     959 100%       command-line-1
     959 100%         main
     959 100%           record-samples
     959 100%             bar
     959 100%               animate-birthday-present
     953  99%                 animate-string
      12   1%                   animate-initialize
      12   1%                     window-width
     139  14%                   sit-for
     139  14%                     sleep-for
     109  11%                   primitive-undo
      15   1%                     markerp
      32   3%                     abs
     693  72%                   animate-step
     693  72%                     animate-place-char
      32   3%                       delete-char
      29   3%                       window-start
      43   4%                       insert-char
     309  32%                       move-to-column
     222  23%                       beginning-of-line
       8   0%                       undo-auto--undoable-change
       8   0%                         undo-auto--boundary-ensure-timer
       8   0%                           run-at-time
       8   0%                             timer-set-function
       8   0%                               timerp
       8   0%                                 vectorp
       6   0%                 capitalize
       6   0%                   load-with-code-conversion
       6   0%                     eval-buffer
       6   0%                       read
       0   0%   ...