Re: More over-engineering

[Stefan Monnier <monnier@iro.umontreal.ca>]

>> Are we really allocating a structure for every Lisp_Object value we pass
>> through the modules API?  Why do that?
> Look at the old thread for Philip's arguments.  As far as I'm
> concerned, we need an indirect pointer to support wide-int
> Lisp_Objects on 32bit platforms.

We don't need it in core so we don't need it in the modules code either.

The only advantage in this respect is that the representation of
Lisp_Object is encapsulated inside Emacs core, so the same module can be
used with a "emacs-32bit-narrow-int" and on "emacs-32bit-wide-int".
The cost for that minor advantage seems way out of proportion.

> Global reference can still be made with the current code. We have
> a reference counting hash-table to keep track of them.

Oh, I see the "make_global_ref" now, thanks.

> Non-global refs are marked explicitely in case the GC misses them.

Why would the GC miss them?


        Stefan


PS: To give an idea of the cost of this, the patch below changes
emacs_value to be synonym of Lisp_Object (well, it doesn't do it right:
it's a quick&dirty patch for now).


diff --git a/modules/mod-test/mod-test.c b/modules/mod-test/mod-test.c
index b0c535c..79f0ae7 100644
--- a/modules/mod-test/mod-test.c
+++ b/modules/mod-test/mod-test.c
@@ -61,7 +61,7 @@ Fmod_test_signal (emacs_env *env, ptrdiff_t nargs, emacs_value args[],
   assert (env->non_local_exit_check (env) == emacs_funcall_exit_return);
   env->non_local_exit_signal (env, env->intern (env, "error"),
 			      env->make_integer (env, 56));
-  return NULL;
+  return 0/* FIXME!  NULL */;
 }
 
 
@@ -73,7 +73,7 @@ Fmod_test_throw (emacs_env *env, ptrdiff_t nargs, emacs_value args[],
   assert (env->non_local_exit_check (env) == emacs_funcall_exit_return);
   env->non_local_exit_throw (env, env->intern (env, "tag"),
 			     env->make_integer (env, 65));
-  return NULL;
+  return 0/* FIXME!  NULL */;
 }
 
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index c75ddeb..bf5178e 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -59,10 +59,6 @@ static DWORD main_thread;
 \f
 /* Memory management.  */
 
-/* An `emacs_value' is just a pointer to a structure holding an
-   internal Lisp object.  */
-struct emacs_value_tag { Lisp_Object v; };
-
 /* Local value objects use a simple fixed-sized block allocation
    scheme without explicit deallocation.  All local values are
    deallocated when the lifetime of their environment ends.  Keep
@@ -71,28 +67,6 @@ struct emacs_value_tag { Lisp_Object v; };
 
 enum { value_frame_size = 512 };
 
-/* A block from which `emacs_value' object can be allocated.  */
-struct emacs_value_frame
-{
-  /* Storage for values.  */
-  struct emacs_value_tag objects[value_frame_size];
-
-  /* Index of the next free value in `objects'.  */
-  int offset;
-
-  /* Pointer to next frame, if any.  */
-  struct emacs_value_frame *next;
-};
-
-/* A structure that holds an initial frame (so that the first local
-   values require no dynamic allocation) and keeps track of the
-   current frame.  */
-static struct emacs_value_storage
-{
-  struct emacs_value_frame initial;
-  struct emacs_value_frame *current;
-} global_storage;
-
 \f
 /* Private runtime and environment members.  */
 
@@ -106,9 +80,7 @@ struct emacs_env_private
   /* Dedicated storage for non-local exit symbol and data so that
      storage is always available for them, even in an out-of-memory
      situation.  */
-  struct emacs_value_tag non_local_exit_symbol, non_local_exit_data;
-
-  struct emacs_value_storage storage;
+  Lisp_Object non_local_exit_symbol, non_local_exit_data;
 };
 
 /* Combine public and private parts in one structure.  This structure
@@ -134,8 +106,7 @@ struct module_fun_env;
 
 static Lisp_Object module_format_fun_env (const struct module_fun_env *);
 static Lisp_Object value_to_lisp (emacs_value);
-static emacs_value allocate_emacs_value (emacs_env *, struct emacs_value_storage *, Lisp_Object);
-static emacs_value lisp_to_value (emacs_env *, Lisp_Object);
+static emacs_value lisp_to_value (Lisp_Object);
 static enum emacs_funcall_exit module_non_local_exit_check (emacs_env *);
 static void check_main_thread (void);
 static void finalize_environment (struct env_storage *);
@@ -149,6 +120,7 @@ static void module_out_of_memory (emacs_env *);
 static void module_reset_handlerlist (const int *);
 static void module_wrong_type (emacs_env *, Lisp_Object, Lisp_Object);
 
+#define mod_NULL (lisp_to_value (Qnil))
 \f
 /* Convenience macros for non-local exit handling.  */
 
@@ -180,7 +152,7 @@ static void module_wrong_type (emacs_env *, Lisp_Object, Lisp_Object);
    ENV parameter.  The function will return NULL if a `throw' is
    caught.  */
 #define MODULE_HANDLE_THROW                                                    \
-  MODULE_SETJMP (CATCHER_ALL, module_handle_throw, NULL)
+  MODULE_SETJMP (CATCHER_ALL, module_handle_throw, mod_NULL)
 
 #define MODULE_SETJMP(handlertype, handlerfunc, retval)			       \
   MODULE_SETJMP_1 (handlertype, handlerfunc, retval,			       \
@@ -261,6 +233,8 @@ static Lisp_Object module_call_func;
       should protect itself from signals and 'throw' in the called
       Emacs functions, by placing the macros MODULE_HANDLE_SIGNALS
       and/or MODULE_HANDLE_THROW right after the above 2 tests.
+   FIXME: Why is it always needed?  What happens if we don't?
+          Why is only one of the two needed rather than always both?
 
    5. Do NOT use 'eassert' for checking validity of user code in the
       module.  Instead, make those checks part of the code, and if the
@@ -289,7 +263,7 @@ module_make_global_ref (emacs_env *env, emacs_value ref)
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+    return mod_NULL;
   MODULE_HANDLE_SIGNALS;
   struct Lisp_Hash_Table *h = XHASH_TABLE (Vmodule_refs_hash);
   Lisp_Object new_obj = value_to_lisp (ref);
@@ -303,7 +277,7 @@ module_make_global_ref (emacs_env *env, emacs_value ref)
       if (refcount > MOST_POSITIVE_FIXNUM)
         {
           module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-          return NULL;
+          return mod_NULL;
         }
       value = make_natnum (refcount);
       set_hash_value_slot (h, i, value);
@@ -313,7 +287,7 @@ module_make_global_ref (emacs_env *env, emacs_value ref)
       hash_put (h, new_obj, make_natnum (1), hashcode);
     }
 
-  return allocate_emacs_value (env, &global_storage, new_obj);
+  return lisp_to_value (new_obj);
 }
 
 static void
@@ -366,8 +340,8 @@ module_non_local_exit_get (emacs_env *env, emacs_value *sym, emacs_value *data)
   struct emacs_env_private *p = env->private_members;
   if (p->pending_non_local_exit != emacs_funcall_exit_return)
     {
-      *sym = &p->non_local_exit_symbol;
-      *data = &p->non_local_exit_data;
+      *sym = lisp_to_value (p->non_local_exit_symbol);
+      *data = lisp_to_value (p->non_local_exit_data);
     }
   return p->pending_non_local_exit;
 }
@@ -404,9 +378,7 @@ module_make_function (emacs_env *env, ptrdiff_t min_arity, ptrdiff_t max_arity,
 		      void *data)
 {
   check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
-  MODULE_HANDLE_SIGNALS;
+  MODULE_HANDLE_SIGNALS;  /* FIXME: Why?  */
 
   if (! (0 <= min_arity
 	 && (max_arity < 0
@@ -434,7 +406,7 @@ module_make_function (emacs_env *env, ptrdiff_t min_arity, ptrdiff_t max_arity,
                                   envobj,
                                   Qargs));
 
-  return lisp_to_value (env, ret);
+  return lisp_to_value (ret);
 }
 
 static emacs_value
@@ -443,7 +415,7 @@ module_funcall (emacs_env *env, emacs_value fun, ptrdiff_t nargs,
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+    return mod_NULL;
   MODULE_HANDLE_SIGNALS;
   MODULE_HANDLE_THROW;
 
@@ -455,7 +427,7 @@ module_funcall (emacs_env *env, emacs_value fun, ptrdiff_t nargs,
   newargs[0] = value_to_lisp (fun);
   for (ptrdiff_t i = 0; i < nargs; i++)
     newargs[1 + i] = value_to_lisp (args[i]);
-  emacs_value result = lisp_to_value (env, Ffuncall (nargs + 1, newargs));
+  emacs_value result = lisp_to_value (Ffuncall (nargs + 1, newargs));
   SAFE_FREE ();
   return result;
 }
@@ -465,9 +437,9 @@ module_intern (emacs_env *env, const char *name)
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+    return mod_NULL;
   MODULE_HANDLE_SIGNALS;
-  return lisp_to_value (env, intern (name));
+  return lisp_to_value (intern (name));
 }
 
 static emacs_value
@@ -475,8 +447,8 @@ module_type_of (emacs_env *env, emacs_value value)
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
-  return lisp_to_value (env, Ftype_of (value_to_lisp (value)));
+    return mod_NULL;
+  return lisp_to_value (Ftype_of (value_to_lisp (value)));
 }
 
 static bool
@@ -517,13 +489,13 @@ module_make_integer (emacs_env *env, intmax_t n)
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+    return mod_NULL;
   if (! (MOST_NEGATIVE_FIXNUM <= n && n <= MOST_POSITIVE_FIXNUM))
     {
       module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return NULL;
+      return mod_NULL;
     }
-  return lisp_to_value (env, make_number (n));
+  return lisp_to_value (make_number (n));
 }
 
 static double
@@ -546,9 +518,9 @@ module_make_float (emacs_env *env, double d)
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+    return mod_NULL;
   MODULE_HANDLE_SIGNALS;
-  return lisp_to_value (env, make_float (d));
+  return lisp_to_value (make_float (d));
 }
 
 static bool
@@ -603,16 +575,15 @@ module_make_string (emacs_env *env, const char *str, ptrdiff_t length)
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+    return mod_NULL;
   MODULE_HANDLE_SIGNALS;
   if (length > STRING_BYTES_BOUND)
     {
       module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return NULL;
+      return mod_NULL;
     }
   Lisp_Object lstr = make_unibyte_string (str, length);
-  return lisp_to_value (env,
-			code_convert_string_norecord (lstr, Qutf_8, false));
+  return lisp_to_value (code_convert_string_norecord (lstr, Qutf_8, false));
 }
 
 static emacs_value
@@ -620,8 +591,8 @@ module_make_user_ptr (emacs_env *env, emacs_finalizer_function fin, void *ptr)
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
-  return lisp_to_value (env, make_user_ptr (fin, ptr));
+    return mod_NULL;
+  return lisp_to_value (make_user_ptr (fin, ptr));
 }
 
 static void *
@@ -707,12 +678,12 @@ module_vec_get (emacs_env *env, emacs_value vec, ptrdiff_t i)
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+    return mod_NULL;
   Lisp_Object lvec = value_to_lisp (vec);
   if (! VECTORP (lvec))
     {
       module_wrong_type (env, Qvectorp, lvec);
-      return NULL;
+      return mod_NULL;
     }
   if (! (0 <= i && i < ASIZE (lvec)))
     {
@@ -720,9 +691,9 @@ module_vec_get (emacs_env *env, emacs_value vec, ptrdiff_t i)
 	module_args_out_of_range (env, lvec, make_number (i));
       else
 	module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return NULL;
+      return mod_NULL;
     }
-  return lisp_to_value (env, AREF (lvec, i));
+  return lisp_to_value (AREF (lvec, i));
 }
 
 static ptrdiff_t
@@ -785,14 +756,15 @@ DEFUN ("module-load", Fmodule_load, Smodule_load, 1, 1, 0,
   return Qt;
 }
 
-DEFUN ("module-call", Fmodule_call, Smodule_call, 2, 2, 0,
+DEFUN ("module-call", Fmodule_call, Smodule_call, 1, MANY, 0,
        doc: /* Internal function to call a module function.
 ENVOBJ is a save pointer to a module_fun_env structure.
 ARGLIST is a list of arguments passed to SUBRPTR.  */)
-  (Lisp_Object envobj, Lisp_Object arglist)
+  (ptrdiff_t nargs, Lisp_Object *args)
 {
+  Lisp_Object envobj = args[0];
   struct module_fun_env *envptr = XSAVE_POINTER (envobj, 0);
-  EMACS_INT len = XFASTINT (Flength (arglist));
+  EMACS_INT len = nargs - 1;
   eassume (0 <= envptr->min_arity);
   if (! (envptr->min_arity <= len
 	 && len <= (envptr->max_arity < 0 ? PTRDIFF_MAX : envptr->max_arity)))
@@ -802,37 +774,28 @@ ARGLIST is a list of arguments passed to SUBRPTR.  */)
   struct env_storage env;
   initialize_environment (&env);
 
-  emacs_value *args = xnmalloc (len, sizeof *args);
-
-  for (ptrdiff_t i = 0; i < len; i++)
-    {
-      args[i] = lisp_to_value (&env.pub, XCAR (arglist));
-      if (! args[i])
-	memory_full (sizeof *args[i]);
-      arglist = XCDR (arglist);
-    }
-
-  emacs_value ret = envptr->subr (&env.pub, len, args, envptr->data);
-  xfree (args);
+  emacs_value ret = envptr->subr (&env.pub, len,
+                                  /* BEWARE!  Here, we assume that Lisp_Object
+                                     and emacs_value have the exact same
+                                     representation.  */
+                                  (emacs_value*) args+1, envptr->data);
 
   switch (env.priv.pending_non_local_exit)
     {
     case emacs_funcall_exit_return:
       finalize_environment (&env);
-      if (ret == NULL)
-	xsignal1 (Qinvalid_module_call, module_format_fun_env (envptr));
       return value_to_lisp (ret);
     case emacs_funcall_exit_signal:
       {
-        Lisp_Object symbol = value_to_lisp (&env.priv.non_local_exit_symbol);
-        Lisp_Object data = value_to_lisp (&env.priv.non_local_exit_data);
+        Lisp_Object symbol = env.priv.non_local_exit_symbol;
+        Lisp_Object data = env.priv.non_local_exit_data;
         finalize_environment (&env);
         xsignal (symbol, data);
       }
     case emacs_funcall_exit_throw:
       {
-        Lisp_Object tag = value_to_lisp (&env.priv.non_local_exit_symbol);
-        Lisp_Object value = value_to_lisp (&env.priv.non_local_exit_data);
+        Lisp_Object tag = env.priv.non_local_exit_symbol;
+        Lisp_Object value = env.priv.non_local_exit_data;
         finalize_environment (&env);
         Fthrow (tag, value);
       }
@@ -864,8 +827,8 @@ module_non_local_exit_signal_1 (emacs_env *env, Lisp_Object sym,
   if (p->pending_non_local_exit == emacs_funcall_exit_return)
     {
       p->pending_non_local_exit = emacs_funcall_exit_signal;
-      p->non_local_exit_symbol.v = sym;
-      p->non_local_exit_data.v = data;
+      p->non_local_exit_symbol = sym;
+      p->non_local_exit_data = data;
     }
 }
 
@@ -877,8 +840,8 @@ module_non_local_exit_throw_1 (emacs_env *env, Lisp_Object tag,
   if (p->pending_non_local_exit == emacs_funcall_exit_return)
     {
       p->pending_non_local_exit = emacs_funcall_exit_throw;
-      p->non_local_exit_symbol.v = tag;
-      p->non_local_exit_data.v = value;
+      p->non_local_exit_symbol = tag;
+      p->non_local_exit_data = value;
     }
 }
 
@@ -915,94 +878,25 @@ module_args_out_of_range (emacs_env *env, Lisp_Object a1, Lisp_Object a2)
 static Lisp_Object
 value_to_lisp (emacs_value v)
 {
-  return v->v;
+  return XIL (v);
 }
 
 /* Convert an internal object to an `emacs_value'.  Allocate storage
    from the environment; return NULL if allocation fails.  */
 static emacs_value
-lisp_to_value (emacs_env *env, Lisp_Object o)
+lisp_to_value (Lisp_Object o)
 {
-  struct emacs_env_private *p = env->private_members;
-  if (p->pending_non_local_exit != emacs_funcall_exit_return)
-    return NULL;
-  return allocate_emacs_value (env, &p->storage, o);
+  return XLI (o);
 }
 
 \f
 /* Memory management.  */
 
-/* Must be called for each frame before it can be used for allocation.  */
-static void
-initialize_frame (struct emacs_value_frame *frame)
-{
-  frame->offset = 0;
-  frame->next = NULL;
-}
-
-/* Must be called for any storage object before it can be used for
-   allocation.  */
-static void
-initialize_storage (struct emacs_value_storage *storage)
-{
-  initialize_frame (&storage->initial);
-  storage->current = &storage->initial;
-}
-
-/* Must be called for any initialized storage object before its
-   lifetime ends.  Free all dynamically-allocated frames.  */
-static void
-finalize_storage (struct emacs_value_storage *storage)
-{
-  struct emacs_value_frame *next = storage->initial.next;
-  while (next != NULL)
-    {
-      struct emacs_value_frame *current = next;
-      next = current->next;
-      free (current);
-    }
-}
-
-/* Allocate a new value from STORAGE and stores OBJ in it.  Return
-   NULL if allocation fails and use ENV for non local exit reporting.  */
-static emacs_value
-allocate_emacs_value (emacs_env *env, struct emacs_value_storage *storage,
-		      Lisp_Object obj)
-{
-  eassert (storage->current);
-  eassert (storage->current->offset < value_frame_size);
-  eassert (! storage->current->next);
-  if (storage->current->offset == value_frame_size - 1)
-    {
-      storage->current->next = malloc (sizeof *storage->current->next);
-      if (! storage->current->next)
-        {
-          module_out_of_memory (env);
-          return NULL;
-        }
-      initialize_frame (storage->current->next);
-      storage->current = storage->current->next;
-    }
-  emacs_value value = storage->current->objects + storage->current->offset;
-  value->v = obj;
-  ++storage->current->offset;
-  return value;
-}
-
 /* Mark all objects allocated from local environments so that they
    don't get garbage-collected.  */
 void
 mark_modules (void)
 {
-  for (Lisp_Object tem = Vmodule_environments; CONSP (tem); tem = XCDR (tem))
-    {
-      struct env_storage *env = XSAVE_POINTER (tem, 0);
-      for (struct emacs_value_frame *frame = &env->priv.storage.initial;
-	   frame != NULL;
-	   frame = frame->next)
-        for (int i = 0; i < frame->offset; ++i)
-          mark_object (frame->objects[i].v);
-    }
 }
 
 \f
@@ -1013,7 +907,6 @@ static void
 initialize_environment (struct env_storage *env)
 {
   env->priv.pending_non_local_exit = emacs_funcall_exit_return;
-  initialize_storage (&env->priv.storage);
   env->pub.size = sizeof env->pub;
   env->pub.private_members = &env->priv;
   env->pub.make_global_ref = module_make_global_ref;
@@ -1051,7 +944,6 @@ initialize_environment (struct env_storage *env)
 static void
 finalize_environment (struct env_storage *env)
 {
-  finalize_storage (&env->priv.storage);
   Vmodule_environments = XCDR (Vmodule_environments);
 }
 
@@ -1152,8 +1044,6 @@ syms_of_module (void)
   Fput (Qinvalid_arity, Qerror_message,
         build_pure_c_string ("Invalid function arity"));
 
-  initialize_storage (&global_storage);
-
   /* Unintern `module-refs-hash' because it is internal-only and Lisp
      code or modules should not access it.  */
   Funintern (Qmodule_refs_hash, Qnil);
diff --git a/src/emacs-module.h b/src/emacs-module.h
index ea5de76..f6e3141 100644
--- a/src/emacs-module.h
+++ b/src/emacs-module.h
@@ -38,7 +38,7 @@ extern "C" {
 typedef struct emacs_env_25 emacs_env;
 
 /* Opaque structure pointer representing an Emacs Lisp value.  */
-typedef struct emacs_value_tag *emacs_value;
+typedef long emacs_value;
 
 enum emacs_arity { emacs_variadic_function = -2 };