Re: master 46f3452b30f: Simplify and speed up make-hash-table argument parsing

Re: master 46f3452b30f: Simplify and speed up make-hash-table argument parsing

[Pip Cet]

Mattias Engdegård via Mailing list for Emacs changes <emacs-diffs@gnu.org> writes:

> branch: master
> commit 46f3452b30f39a69f610faab58c1490b34dd367d
> Author: Mattias Engdegård <mattiase@acm.org>
> Commit: Mattias Engdegård <mattiase@acm.org>
>
>     Simplify and speed up make-hash-table argument parsing
>
>     * src/fns.c (get_key_arg): Remove.
>     (Fmake_hash_table): Traverse argument list once only.  Don't allocate a
>     helper array.  Use faster comparisons.
> ---
>  src/fns.c | 112 +++++++++++++++++++++++---------------------------------------
>  1 file changed, 42 insertions(+), 70 deletions(-)
>
> diff --git a/src/fns.c b/src/fns.c
> index 80794bc73a0..80ae554c86e 100644
> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -4637,30 +4637,6 @@ next_almost_prime (EMACS_INT n)
>        return n;
>  }
>
> -
> -/* Find KEY in ARGS which has size NARGS.  Don't consider indices for
> -   which USED[I] is non-zero.  If found at index I in ARGS, set
> -   USED[I] and USED[I + 1] to 1, and return I + 1.  Otherwise return
> -   0.  This function is used to extract a keyword/argument pair from
> -   a DEFUN parameter list.  */
> -
> -static ptrdiff_t
> -get_key_arg (Lisp_Object key, ptrdiff_t nargs, Lisp_Object *args, char *used)
> -{
> -  ptrdiff_t i;
> -
> -  for (i = 1; i < nargs; i++)
> -    if (!used[i - 1] && EQ (args[i - 1], key))
> -      {
> -	used[i - 1] = 1;
> -	used[i] = 1;
> -	return i;
> -      }
> -
> -  return 0;
> -}
> -
> -

This (removed) code used to accept calls such as

(make-hash-table :rehash-size :test 'eq)

which doesn't make sense, so we definitely should change something here.

>  /* Return a Lisp vector which has the same contents as VEC but has
>     at least INCR_MIN more entries, where INCR_MIN is positive.
>     If NITEMS_MAX is not -1, do not grow the vector to be any larger
> @@ -5762,32 +5738,43 @@ and ignored.
>  usage: (make-hash-table &rest KEYWORD-ARGS)  */)
>    (ptrdiff_t nargs, Lisp_Object *args)
>  {
> -  USE_SAFE_ALLOCA;
> +  Lisp_Object test_arg = Qnil;
> +  Lisp_Object weakness_arg = Qnil;
> +  Lisp_Object size_arg = Qnil;
> +  Lisp_Object purecopy_arg = Qnil;
> +
> +  if (nargs & 1)
> +    error ("Odd number of arguments");
> +  while (nargs >= 2)
> +    {
> +      Lisp_Object arg = maybe_remove_pos_from_symbol (args[--nargs]);
> +      Lisp_Object kw = maybe_remove_pos_from_symbol (args[--nargs]);
> +      if (BASE_EQ (kw, QCtest))

Is this really faster than not removing the position from the symbol in
the first place? (I must confess I don't like using BASE_EQ, we should
just use 'eassume' and let the compiler choose the faster
implementation, but that doesn't actually appear to work too well in
practice).

> +	test_arg = arg;
> +      else if (BASE_EQ (kw, QCweakness))
> +	weakness_arg = arg;
> +      else if (BASE_EQ (kw, QCsize))
> +	size_arg = arg;
> +      else if (BASE_EQ (kw, QCpurecopy))
> +	purecopy_arg = arg;
> +      else if (BASE_EQ (kw, QCrehash_threshold) || BASE_EQ (kw, QCrehash_size))
> +	;  /* ignore obsolete keyword arguments */
> +      else
> +	signal_error ("Invalid keyword argument", kw);
> +    }

The new code accepts duplicate arguments, such as

(make-hash-table :test 'eq :test 'eq)

Is that intentional, harmless, or undesirable?

Anyway, it'll be even simpler once we remove purespace :-)

Pip

Re: master 46f3452b30f: Simplify and speed up make-hash-table argument parsing

[Mattias Engdegård]

21 aug. 2024 kl. 15.39 skrev Pip Cet <pipcet@protonmail.com>:

> This (removed) code used to accept calls such as
> 
> (make-hash-table :rehash-size :test 'eq)
> 
> which doesn't make sense, so we definitely should change something here.

Indeed, this is a change long overdue. It is also unfinished business from the recent hash-table reform; somehow it got lost in the shuffle.

> Is this really faster than not removing the position from the symbol in
> the first place?

Yes, because of the number of subsequent branches affected.

> The new code accepts duplicate arguments, such as
> 
> (make-hash-table :test 'eq :test 'eq)
> 
> Is that intentional, harmless, or undesirable?

Quite harmless and definitely not worth spending any cycles detecting, especially not at runtime.

> Anyway, it'll be even simpler once we remove purespace

Looking forward to it!