MPS command line parameters

MPS command line parameters

[Helmut Eller]

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

The patches below add a command line option to configure MPS.  It can be
used like

  emacs --igc-param pause_time=INF \
        --igc-param gen_params=64000k80%,256000k20% \
        --igc-param commit_limit=1000000000

WDYT?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Introduce-a-struct-igc_params.patch --]
[-- Type: text/x-diff, Size: 5261 bytes --]

From 63010af19ff11e77aee0846e7b795cc4df897f4a Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Fri, 16 Aug 2024 08:21:25 +0200
Subject: [PATCH 1/4] Introduce a struct igc_params

* src/igc.h (struct igc_params): New.
(init_igc): Take igc_params as argument.
* src/emacs.c (parse_igc_params, parse_igc_param): New.
(main): Parse igc_params before init_igc.
(usage_message, standard_args): Include --igc-param.
* src/igc.c (make_arena): Use igc_params to configure pause_time.
(init_igc, make_igc): Pass igc_params along.
---
 src/emacs.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/igc.c   | 14 +++++++++-----
 src/igc.h   | 11 ++++++++++-
 3 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/src/emacs.c b/src/emacs.c
index 88b96ca7761..ced3f723919 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -291,6 +291,11 @@ #define MAIN_PROGRAM
 --fingerprint               output fingerprint and exit\n\
 ",
 #endif
+#if HAVE_MPS
+    "\
+--igc-param PARAM=VALUE     set IGC paramater PARAM to VALUE\n\
+"
+#endif
 #if SECCOMP_USABLE
     "\
 --seccomp=FILE              read Seccomp BPF filter from FILE\n\
@@ -730,6 +735,42 @@ argmatch (char **argv, int argc, const char *sstr, const char *lstr,
     }
 }
 
+#ifdef HAVE_MPS
+static void
+parse_igc_param (const char *val, struct igc_params *params)
+{
+  int n = 0;
+  if ((sscanf (val, "pause_time=%lf%n", &params->pause_time.seconds, &n) == 1)
+      && val[n] == '\0' && params->pause_time.seconds >= 0)
+    {
+      params->pause_time.dflt = false;
+      return;
+    }
+  fprintf (stderr, "Invalid igc_param: %s\n", val);
+  exit (1);
+}
+
+static struct igc_params
+parse_igc_params (int argc, char **argv)
+{
+  struct igc_params params = {
+    .pause_time = { .dflt = true },
+    .gen_params = { .dflt = true },
+  };
+  for (int skip = 0; skip != argc;)
+    {
+      char *val = NULL;
+      if (argmatch (argv, argc, "-igc-param", "--igc-param", 11, &val, &skip))
+	parse_igc_param (val, &params);
+      else if (argmatch (argv, argc, "--", NULL, 0, NULL, &skip))
+	break;
+      else
+	skip++;
+    }
+  return params;
+}
+#endif
+
 #if !defined HAVE_ANDROID || defined ANDROID_STUBIFY
 
 /* Find a name (absolute or relative) of the Emacs executable whose
@@ -1421,7 +1462,8 @@ main (int argc, char **argv)
   init_signals ();
 
 #ifdef HAVE_MPS
-  init_igc ();
+  const struct igc_params params = parse_igc_params (argc, argv);
+  init_igc (&params);
 #endif
 
   /* This is needed early because load_pdump can call 'float-time' (via
@@ -2051,6 +2093,15 @@ main (int argc, char **argv)
   init_module_assertions (module_assertions);
 #endif
 
+#ifdef HAVE_MPS
+  {
+    char *arg;
+    while (argmatch (argv, argc, "-igc-param", "--igc-param", 11,
+		     &arg, &skip_args))
+      ;
+  }
+#endif
+
 #ifdef HAVE_NS
   if (!noninteractive)
     {
@@ -2681,6 +2732,9 @@ main (int argc, char **argv)
   { "-no-build-details", "--no-build-details", 63, 0 },
 #ifdef HAVE_MODULES
   { "-module-assertions", "--module-assertions", 62, 0 },
+#endif
+#ifdef HAVE_MPS
+  {"-igc-param", "--igc-param", 61, 1 },
 #endif
   /* -d must come last before the options handled in startup.el.  */
   { "-d", "--display", 60, 1 },
diff --git a/src/igc.c b/src/igc.c
index f069a2becc9..c294eae747e 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -4448,12 +4448,16 @@ DEFUN ("igc--set-commit-limit", Figc__set_commit_limit,
   return Qnil;
 }
 
+static_assert (sizeof (struct igc_gen_param) == sizeof (mps_gen_param_s));
+
 static void
-make_arena (struct igc *gc)
+make_arena (struct igc *gc, const struct igc_params *params)
 {
   mps_res_t res;
   MPS_ARGS_BEGIN (args)
   {
+    if (!params->pause_time.dflt)
+      MPS_ARGS_ADD (args, MPS_KEY_PAUSE_TIME, params->pause_time.seconds);
     res = mps_arena_create_k (&gc->arena, mps_arena_class_vm (), args);
   }
   MPS_ARGS_END (args);
@@ -4534,10 +4538,10 @@ make_pool_amcz (struct igc *gc, mps_fmt_t fmt)
 }
 
 static struct igc *
-make_igc (void)
+make_igc (const struct igc_params *params)
 {
   struct igc *gc = xzalloc (sizeof *gc);
-  make_arena (gc);
+  make_arena (gc, params);
 
   /* We cannot let the GC run until at least all staticpros haven been
      processed. Otherwise we might allocate objects that are not
@@ -4967,11 +4971,11 @@ DEFUN ("igc--remove-extra-dependency", Figc__remove_extra_dependency,
  ***********************************************************************/
 
 void
-init_igc (void)
+init_igc (const struct igc_params *params)
 {
   /* Returns previous handler.  */
   (void) mps_lib_assert_fail_install (igc_assert_fail);
-  global_igc = make_igc ();
+  global_igc = make_igc (params);
   add_main_thread ();
   set_state (IGC_STATE_USABLE_PARKED);
 }
diff --git a/src/igc.h b/src/igc.h
index 00e37b6c952..586db8ce77f 100644
--- a/src/igc.h
+++ b/src/igc.h
@@ -60,8 +60,17 @@ #define EMACS_IGC_H
 
 #ifdef HAVE_MPS
 
+struct igc_params
+{
+  struct
+  {
+    bool dflt;
+    double seconds; /* in seconds; valid values in [0..INF] */
+  } pause_time;
+};
+
 void igc_break (void);
-void init_igc (void);
+void init_igc (const struct igc_params *);
 void syms_of_igc (void);
 void *igc_thread_add (struct thread_state *ts);
 void igc_thread_remove (void **info);
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-gen_params-to-igc_params.patch --]
[-- Type: text/x-diff, Size: 3641 bytes --]

From e732965529ce3b29de2d211349c033f4b5724a23 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Fri, 16 Aug 2024 08:28:35 +0200
Subject: [PATCH 2/4] Add gen_params to igc_params.

* src/igc.h (struct igc_gen_param): New.
(struct igc_params): Add gen_params field.
* src/emacs.c (parse_igc_gen_params): New.
(parse_igc_param): Use it.
* src/igc.c (make_arena): Use igc_params to configure the generation
chain.
---
 src/emacs.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 src/igc.c   | 11 +++++++++--
 src/igc.h   | 12 ++++++++++++
 3 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/src/emacs.c b/src/emacs.c
index ced3f723919..b2d12b61967 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -736,6 +736,45 @@ argmatch (char **argv, int argc, const char *sstr, const char *lstr,
 }
 
 #ifdef HAVE_MPS
+static void
+parse_igc_gen_params (const char *string, struct igc_params *params)
+{
+  size_t count = 0;
+  struct igc_gen_param *array = NULL;
+  for (const char *s = string;;)
+    {
+      int n = 0;
+      size_t capacity, mortality;
+      if ((sscanf (s, "%zuk%zu%%%n", &capacity, &mortality, &n) == 2)
+	  && mortality <= 100)
+	{
+	  struct igc_gen_param p = { .capacity = capacity,
+				     .mortality = (double)mortality / 100.0 };
+	  count++;
+	  array = xrealloc (array, sizeof (array[0]) * count);
+	  array[count - 1] = p;
+	  if (s[n] == ',')
+	    {
+	      s = &s[n + 1];
+	      continue;
+	    }
+	  else if (s[n] == '\0')
+	    {
+	      params->gen_params.dflt = false;
+	      params->gen_params.gen_count = count;
+	      params->gen_params.gen_params = array;
+	      return;
+	    }
+	  else
+	    break;
+	}
+      else
+	break;
+    }
+  fprintf (stderr, "Invalid value gen_params: %s\n", string);
+  exit (1);
+}
+
 static void
 parse_igc_param (const char *val, struct igc_params *params)
 {
@@ -746,6 +785,11 @@ parse_igc_param (const char *val, struct igc_params *params)
       params->pause_time.dflt = false;
       return;
     }
+
+  const char *key = "gen_params=";
+  if (strncmp (val, key, strlen (key)) == 0)
+    return parse_igc_gen_params (val + strlen (key), params);
+
   fprintf (stderr, "Invalid igc_param: %s\n", val);
   exit (1);
 }
diff --git a/src/igc.c b/src/igc.c
index c294eae747e..77f897bfdca 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -4463,8 +4463,15 @@ make_arena (struct igc *gc, const struct igc_params *params)
   MPS_ARGS_END (args);
   IGC_CHECK_RES (res);
 
-  mps_gen_param_s gens[] = { { 128000, 0.8 }, { 5 * 128000, 0.4 } };
-  res = mps_chain_create (&gc->chain, gc->arena, ARRAYELTS (gens), gens);
+  mps_gen_param_s dflt_gens[] = { { 128000, 0.8 }, { 5 * 128000, 0.4 } };
+  mps_gen_param_s *gens = dflt_gens;
+  size_t ngens = ARRAYELTS (dflt_gens);
+  if (!params->gen_params.dflt)
+    {
+      ngens = params->gen_params.gen_count;
+      gens = (mps_gen_param_s *)params->gen_params.gen_params;
+    }
+  res = mps_chain_create (&gc->chain, gc->arena, ngens, gens);
   IGC_CHECK_RES (res);
 }
 
diff --git a/src/igc.h b/src/igc.h
index 586db8ce77f..716069adceb 100644
--- a/src/igc.h
+++ b/src/igc.h
@@ -60,6 +60,12 @@ #define EMACS_IGC_H
 
 #ifdef HAVE_MPS
 
+struct igc_gen_param
+{
+  size_t capacity;	  /* in kilobytes */
+  double mortality;	  /* initial estimate; valid values in [0..1] */
+};
+
 struct igc_params
 {
   struct
@@ -67,6 +73,12 @@ #define EMACS_IGC_H
     bool dflt;
     double seconds; /* in seconds; valid values in [0..INF] */
   } pause_time;
+  struct
+  {
+    bool dflt;
+    size_t gen_count;
+    struct igc_gen_param *gen_params;
+  } gen_params;
 };
 
 void igc_break (void);
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Add-commit_limit-to-igc_params.patch --]
[-- Type: text/x-diff, Size: 2187 bytes --]

From 185135e876ea29511e72db090e91636dc0402344 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Fri, 16 Aug 2024 09:05:06 +0200
Subject: [PATCH 3/4] Add commit_limit to igc_params

* src/igc.h (struct igc_params): Add commit_limit field.
* src/igc.c (make_arena): Use igc_params to configure commit_limit.
* src/emacs.c (parse_igc_params, parse_igc_param): Parse commit_limit.
---
 src/emacs.c | 9 +++++++++
 src/igc.c   | 2 ++
 src/igc.h   | 5 +++++
 3 files changed, 16 insertions(+)

diff --git a/src/emacs.c b/src/emacs.c
index b2d12b61967..777ca703079 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -790,6 +790,14 @@ parse_igc_param (const char *val, struct igc_params *params)
   if (strncmp (val, key, strlen (key)) == 0)
     return parse_igc_gen_params (val + strlen (key), params);
 
+  if ((sscanf (val, "commit_limit=%zu%n", &params->commit_limit.bytes, &n)
+       == 1)
+      && val[n] == '\0')
+    {
+      params->commit_limit.dflt = false;
+      return;
+    }
+
   fprintf (stderr, "Invalid igc_param: %s\n", val);
   exit (1);
 }
@@ -800,6 +808,7 @@ parse_igc_params (int argc, char **argv)
   struct igc_params params = {
     .pause_time = { .dflt = true },
     .gen_params = { .dflt = true },
+    .commit_limit = { .dflt = true },
   };
   for (int skip = 0; skip != argc;)
     {
diff --git a/src/igc.c b/src/igc.c
index 77f897bfdca..89e767ad79f 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -4458,6 +4458,8 @@ make_arena (struct igc *gc, const struct igc_params *params)
   {
     if (!params->pause_time.dflt)
       MPS_ARGS_ADD (args, MPS_KEY_PAUSE_TIME, params->pause_time.seconds);
+    if (!params->commit_limit.dflt)
+      MPS_ARGS_ADD (args, MPS_KEY_COMMIT_LIMIT, params->commit_limit.bytes);
     res = mps_arena_create_k (&gc->arena, mps_arena_class_vm (), args);
   }
   MPS_ARGS_END (args);
diff --git a/src/igc.h b/src/igc.h
index 716069adceb..abe2452f625 100644
--- a/src/igc.h
+++ b/src/igc.h
@@ -79,6 +79,11 @@ #define EMACS_IGC_H
     size_t gen_count;
     struct igc_gen_param *gen_params;
   } gen_params;
+  struct
+  {
+    bool dflt;
+    size_t bytes;
+  } commit_limit;
 };
 
 void igc_break (void);
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Remove-igc-set-commit-limit.patch --]
[-- Type: text/x-diff, Size: 3076 bytes --]

From 4a7915668552bc029d43b4c6752274c527be7644 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Fri, 16 Aug 2024 09:15:58 +0200
Subject: [PATCH 4/4] Remove igc--set-commit-limit

The commit_limit can now be set with the --igc-param command line
option.

* src/igc.c (Figc__set_commit_limit, mps_res_to_string): Deleted.
* test/src/igc-tests.el (set-commit-limit-test): Deleted.
---
 src/igc.c             | 38 --------------------------------------
 test/src/igc-tests.el | 12 ------------
 2 files changed, 50 deletions(-)

diff --git a/src/igc.c b/src/igc.c
index 89e767ad79f..82c5886a8a5 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -4411,43 +4411,6 @@ igc_external_header (struct igc_header *h)
   return header_exthdr (h);
 }
 
-static const char *
-mps_res_to_string (mps_res_t res)
-{
-  switch (res)
-    {
-#define RES_CASE(prefix, id, doc)                                            \
-  case prefix##id:                                                           \
-    return #prefix #id " " doc;
-      _mps_RES_ENUM (RES_CASE, MPS_RES_);
-#undef RES_CASE
-    default:
-      return NULL;
-    }
-}
-
-DEFUN ("igc--set-commit-limit", Figc__set_commit_limit,
-       Sigc__set_commit_limit, 1, 1, 0,
-       doc: /* Set the arena commit limit to LIMIT.
-LIMIT can be an integer (number of bytes) or nil (no limit).
-
-Do NOT use this for anything but testing, unless you
-really know what you are doing!  */)
-  (Lisp_Object limit)
-{
-  size_t nbytes
-      = NILP (limit) ? ~0 : check_uinteger_max (limit, SIZE_MAX - 1);
-  mps_res_t err = mps_arena_commit_limit_set (global_igc->arena, nbytes);
-  if (err != MPS_RES_OK)
-    {
-      const char *msg = mps_res_to_string (err);
-      xsignal3 (Qerror,
-		intern_c_string (Sigc__set_commit_limit.s.symbol_name),
-		make_fixnum (err), msg ? build_string (msg) : Qnil);
-    }
-  return Qnil;
-}
-
 static_assert (sizeof (struct igc_gen_param) == sizeof (mps_gen_param_s));
 
 static void
@@ -4995,7 +4958,6 @@ syms_of_igc (void)
   defsubr (&Sigc_info);
   defsubr (&Sigc__roots);
   defsubr (&Sigc__collect);
-  defsubr (&Sigc__set_commit_limit);
   defsubr (&Sigc__add_extra_dependency);
   defsubr (&Sigc__remove_extra_dependency);
   DEFSYM (Qambig, "ambig");
diff --git a/test/src/igc-tests.el b/test/src/igc-tests.el
index 59a0cb83909..2d0e296f06c 100644
--- a/test/src/igc-tests.el
+++ b/test/src/igc-tests.el
@@ -1,15 +1,3 @@
 ;;; igc-tests.el --- tests for src/igc.c -*- lexical-binding: t -*-
 
 (require 'ert)
-
-(ert-deftest set-commit-limit-test ()
-  (should (equal (igc--set-commit-limit (ash 1 30)) nil))
-  (should (equal (assoc-string "commit-limit" (igc-info))
-                 '("commit-limit" 1 1073741824 0)))
-  (should-error (igc--set-commit-limit -1)
-                :type 'args-out-of-range)
-  (should-error (igc--set-commit-limit (- (ash 1 64) 1))
-                :type 'args-out-of-range)
-  (should (equal (igc--set-commit-limit nil) nil))
-  (should (equal (assoc-string "commit-limit" (igc-info))
-                 '("commit-limit" 1 -1 0))))
-- 
2.39.2

Re: MPS command line parameters

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> CC: emacs-devel@gnu.org, Gerd Möllmann
>  <gerd.moellmann@gmail.com>
> Date: Fri, 16 Aug 2024 09:32:14 +0200
> 
> The patches below add a command line option to configure MPS.  It can be
> used like
> 
>   emacs --igc-param pause_time=INF \
>         --igc-param gen_params=64000k80%,256000k20% \
>         --igc-param commit_limit=1000000000

Is there any way of changing these parameters when Emacs is already
running?  It is quite un-Emacsy to allow some configuration only via
the command line.  Also, relatively inconvenient, since you must
restart Emacs each time you want to change these parameters.

Thanks.

Re: MPS command line parameters

[Pip Cet]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> From: Helmut Eller <eller.helmut@gmail.com>
>> CC: emacs-devel@gnu.org, Gerd Möllmann
>>  <gerd.moellmann@gmail.com>
>> Date: Fri, 16 Aug 2024 09:32:14 +0200
>>
>> The patches below add a command line option to configure MPS.  It can be
>> used like
>>
>>   emacs --igc-param pause_time=INF \
>>         --igc-param gen_params=64000k80%,256000k20% \
>>         --igc-param commit_limit=1000000000
>
> Is there any way of changing these parameters when Emacs is already
> running?

For most parameters, yes, but the generation chain parameters can only
be set when the pool using them is created.  It's possible, of course,
to create a new pool for new allocations when the parameters change and
use that, but then we'd have to keep track of obsolete pools and that
would complicate things further.

> It is quite un-Emacsy to allow some configuration only via
> the command line.  Also, relatively inconvenient, since you must
> restart Emacs each time you want to change these parameters.

Unfortunately, the generation size settings have a huge influence on
performance, so we might have to implement the pool-switching thing
after all...

Pip

Re: MPS command line parameters

[Helmut Eller]

On Fri, Aug 16 2024, Eli Zaretskii wrote:

>> The patches below add a command line option to configure MPS.  It can be
>> used like
>> 
>>   emacs --igc-param pause_time=INF \
>>         --igc-param gen_params=64000k80%,256000k20% \
>>         --igc-param commit_limit=1000000000
>
> Is there any way of changing these parameters when Emacs is already
> running? It is quite un-Emacsy to allow some configuration only via
> the command line.  Also, relatively inconvenient, since you must
> restart Emacs each time you want to change these parameters.

pause_time and commit_limit could be set at runtime; gen_params not.

Re: MPS command line parameters

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: pipcet@protonmail.com,  emacs-devel@gnu.org,  gerd.moellmann@gmail.com
> Date: Fri, 16 Aug 2024 10:59:48 +0200
> 
> On Fri, Aug 16 2024, Eli Zaretskii wrote:
> 
> >> The patches below add a command line option to configure MPS.  It can be
> >> used like
> >> 
> >>   emacs --igc-param pause_time=INF \
> >>         --igc-param gen_params=64000k80%,256000k20% \
> >>         --igc-param commit_limit=1000000000
> >
> > Is there any way of changing these parameters when Emacs is already
> > running? It is quite un-Emacsy to allow some configuration only via
> > the command line.  Also, relatively inconvenient, since you must
> > restart Emacs each time you want to change these parameters.
> 
> pause_time and commit_limit could be set at runtime; gen_params not.

It would be good to have variables/functions to change those
parameters that can be modified during a session.

Re: MPS command line parameters

[Pip Cet]

"Helmut Eller" <eller.helmut@gmail.com> writes:

> On Fri, Aug 16 2024, Eli Zaretskii wrote:
>
>>> The patches below add a command line option to configure MPS.  It can be
>>> used like
>>>
>>>   emacs --igc-param pause_time=INF \
>>>         --igc-param gen_params=64000k80%,256000k20% \
>>>         --igc-param commit_limit=1000000000
>>
>> Is there any way of changing these parameters when Emacs is already
>> running? It is quite un-Emacsy to allow some configuration only via
>> the command line.  Also, relatively inconvenient, since you must
>> restart Emacs each time you want to change these parameters.
>
> pause_time and commit_limit could be set at runtime; gen_params not.

I haven't forgotten about this.  Would it be okay to install a patch to
set the generation chain params and the grain size at startup time, and
leave the code in to set the other parameters at runtime?

Pip

Re: MPS command line parameters

[Helmut Eller]

On Wed, Aug 21 2024, Pip Cet wrote:

>> pause_time and commit_limit could be set at runtime; gen_params not.
>
> I haven't forgotten about this.  Would it be okay to install a patch to
> set the generation chain params and the grain size at startup time, and
> leave the code in to set the other parameters at runtime?

Of course it would be okay.  Though, I think those parameters will
almost exclusively be set on startup before running a benchmark.