bug#41242: Port feature/native-comp to Windows - Reduce the number of files probed when finding a lisp file.

[Andrea Corallo]

Hi Nico,

my comments on "Reduce the number of files probed when finding a lisp
file.", the full patch attached in case somebody likes to engage.

> diff --git a/src/lread.c b/src/lread.c
> index 46725d9b0f..aaf4ad1a37 100644
> --- a/src/lread.c
> +++ b/src/lread.c
> @@ -1056,33 +1056,25 @@ This uses the variables `load-suffixes' and 
> `load-file-rep-suffixes'.  */)
>      {
>        Lisp_Object exts = Vload_file_rep_suffixes;
>        Lisp_Object suffix = XCAR (suffixes);
> -      FOR_EACH_TAIL (exts)
> -     lst = Fcons (concat2 (suffix, XCAR (exts)), lst);
> +      if (false
> +#ifdef HAVE_MODULES
> +          || strcmp (MODULES_SUFFIX, SSDATA (suffix)) == 0
> +#ifdef MODULES_SECONDARY_SUFFIX
> +          || strcmp (MODULES_SECONDARY_SUFFIX, SSDATA (suffix)) == 0
> +#endif
> +#endif
> +#ifdef HAVE_NATIVE_COMP
> +          || strcmp (NATIVE_ELISP_SUFFIX, SSDATA (suffix)) == 0
> +#endif
> +          )

We can also use NATIVE_COMP_FLAG instead of HAVE_NATIVE_COMP to limit
the macrology when we are not forced to ifdef out code that involves
data structures only defined with native-comp.

> +     lst = Fcons (suffix, lst);
> +      else
> +        FOR_EACH_TAIL (exts)
> +          lst = Fcons (concat2 (suffix, XCAR (exts)), lst);
>      }
>    return Fnreverse (lst);
>  }
>
> -static Lisp_Object
> -effective_load_path (void)
> -{
> -#ifndef HAVE_NATIVE_COMP
> -  return Vload_path;
> -#else
> -  Lisp_Object lp = Vload_path;
> -  Lisp_Object new_lp = Qnil;
> -  FOR_EACH_TAIL (lp)
> -    {
> -      Lisp_Object el = XCAR (lp);
> -      new_lp =
> -     Fcons (concat2 (Ffile_name_as_directory (el),
> -                     Vcomp_native_path_postfix),
> -            new_lp);
> -      new_lp = Fcons (el, new_lp);
> -    }
> -  return Fnreverse (new_lp);
> -#endif
> -}
> -
>  /* Return true if STRING ends with SUFFIX.  */
>  bool
>  suffix_p (Lisp_Object string, const char *suffix)
> @@ -1217,6 +1209,9 @@ Return t if the file exists and loads successfully.  */)
>  #ifdef MODULES_SECONDARY_SUFFIX
>                || suffix_p (file, MODULES_SECONDARY_SUFFIX)
>  #endif
> +#endif
> +#ifdef HAVE_NATIVE_COMP
> +              || suffix_p (file, NATIVE_ELISP_SUFFIX)
>  #endif
>             )

Same as previous comment.

>           must_suffix = Qnil;
> @@ -1236,8 +1231,8 @@ Return t if the file exists and loads successfully.  */)
>       }
>
>        fd =
> -     openp (effective_load_path (), file, suffixes, &found, Qnil,
> -            load_prefer_newer);
> +     openp (Vload_path, file, true, suffixes, &found, Qnil,
> +               load_prefer_newer);
>      }
>
>    if (fd == -1)
> @@ -1600,7 +1595,7 @@ directories, make sure the PREDICATE function returns 
> `dir-ok' for them.  */)
>    (Lisp_Object filename, Lisp_Object path, Lisp_Object suffixes, Lisp_Object 
> predicate)
>  {
>    Lisp_Object file;
> -  int fd = openp (path, filename, suffixes, &file, predicate, false);
> +  int fd = openp (path, filename, Qnil, suffixes, &file, predicate, false);
                                     ^^^
                                     false?

>    if (NILP (predicate) && fd >= 0)
>      emacs_close (fd);
>    return file;
> @@ -1611,6 +1606,9 @@ directories, make sure the PREDICATE function returns 
> `dir-ok' for them.  */)
>     On success, return a file descriptor (or 1 or -2 as described below).
>     On failure, return -1 and set errno.
>
> +   If ADD_ELN_DIR is nonzero, use the `comp-native-path-postfix'
> +   variable to change the searched path if the suffix is .eln.
> +

Is there a specific reason to add a parameter to exclude/includes elns?

I'd say we want to have an homogeneous behavior across all call sites
no?

>     SUFFIXES is a list of strings containing possible suffixes.
>     The empty suffix is automatically added if the list is empty.
>
> @@ -1633,8 +1631,9 @@ directories, make sure the PREDICATE function returns 
> `dir-ok' for them.  */)
>     or if a non-nil and non-t PREDICATE is specified.  */
>
>  int
> -openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
> -       Lisp_Object *storeptr, Lisp_Object predicate, bool newer)
> +openp (Lisp_Object path, Lisp_Object str, bool add_eln_dir,
> +       Lisp_Object suffixes, Lisp_Object *storeptr, Lisp_Object predicate,
> +       bool newer)
>  {
>    ptrdiff_t fn_size = 100;
>    char buf[100];
> @@ -1643,7 +1642,7 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object 
> suffixes,
>    ptrdiff_t want_length;
>    Lisp_Object filename;
>    Lisp_Object string, tail, encoded_fn, save_string;
> -  ptrdiff_t max_suffix_len = 0;
> +  ptrdiff_t max_extra_len = 0;
>    int last_errno = ENOENT;
>    int save_fd = -1;
>    USE_SAFE_ALLOCA;
> @@ -1658,8 +1657,15 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object 
> suffixes,
>    FOR_EACH_TAIL_SAFE (tail)
>      {
>        CHECK_STRING_CAR (tail);
> -      max_suffix_len = max (max_suffix_len,
> -                         SBYTES (XCAR (tail)));
> +      char * suf = SSDATA (XCAR (tail));
              ^^
              no space

> +      ptrdiff_t len = SBYTES (XCAR (tail));
> +      if (add_eln_dir && strcmp (NATIVE_ELISP_SUFFIX, suf) == 0)
> +        {
> +          CHECK_STRING (Vcomp_native_path_postfix);
> +          len += 2;
> +          len += SBYTES (Vcomp_native_path_postfix);
> +        }
> +      max_extra_len = max (max_extra_len, len);
>      }
>
>    string = filename = encoded_fn = save_string = Qnil;
> @@ -1677,7 +1683,7 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object 
> suffixes,
>       executable. */
>    FOR_EACH_TAIL_SAFE (path)
>     {
> -    ptrdiff_t baselen, prefixlen;
> +    ptrdiff_t dirnamelen, prefixlen, basenamewext_len;
>
>      if (EQ (path, just_use_str))
>        filename = str;
> @@ -1694,22 +1700,27 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object 
> suffixes,
>         continue;
>        }
>
> +
>      /* Calculate maximum length of any filename made from
>         this path element/specified file name and any possible suffix.  */
> -    want_length = max_suffix_len + SBYTES (filename);
> +    want_length = max_extra_len + SBYTES (filename);
>      if (fn_size <= want_length)
>        {
>       fn_size = 100 + want_length;
>       fn = SAFE_ALLOCA (fn_size);
>        }
>
> +    Lisp_Object dirnamewslash = Ffile_name_directory (filename);
> +    Lisp_Object basenamewext = Ffile_name_nondirectory (filename);
> +
>      /* Copy FILENAME's data to FN but remove starting /: if any.  */
> -    prefixlen = ((SCHARS (filename) > 2
> -               && SREF (filename, 0) == '/'
> -               && SREF (filename, 1) == ':')
> +    prefixlen = ((SCHARS (dirnamewslash) > 2
> +               && SREF (dirnamewslash, 0) == '/'
> +               && SREF (dirnamewslash, 1) == ':')
>                ? 2 : 0);
> -    baselen = SBYTES (filename) - prefixlen;
> -    memcpy (fn, SDATA (filename) + prefixlen, baselen);
> +    dirnamelen = SBYTES (dirnamewslash) - prefixlen;
> +    basenamewext_len = SBYTES (basenamewext);
> +    memcpy (fn, SDATA (dirnamewslash) + prefixlen, dirnamelen);
>
>      /* Loop over suffixes.  */
>      AUTO_LIST1 (empty_string_only, empty_unibyte_string);
> @@ -1719,10 +1730,24 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object 
> suffixes,
>       Lisp_Object suffix = XCAR (tail);
>       ptrdiff_t fnlen, lsuffix = SBYTES (suffix);
>       Lisp_Object handler;
> +        bool add_native_comp_dir = add_eln_dir
> +          && (strcmp (SSDATA (suffix), NATIVE_ELISP_SUFFIX) == 0);

I think this should be optimized for non native comp builds with a
NATIVE_COMP_FLAG && in front.

> +        ptrdiff_t lmiddledir = add_native_comp_dir
> +            ? SBYTES (Vcomp_native_path_postfix) + 1 : 0;
> +
> +        if (add_native_comp_dir)
> +          {
> +            memcpy (fn + dirnamelen, SDATA (Vcomp_native_path_postfix),
> +                   lmiddledir - 1);

Vcomp_native_path_postfix is only defined with HAVE_NATIVE_COMP so I
guess this breaks the stock build and my previous suggestion is not
applicable.

> +            fn[dirnamelen+(lmiddledir-1)] = '/';

Spaces around operators + -.

I could not compile this patch because non all the calls to openp has
been updated for the new parameter (my question on adding this stands).

In general please recall to check the stock build when working on
infrastructure integration, it's quite easy to break.

Generally speaking I think the behavior we want to have is that when a
.eln file is specified this is loaded without re-adding
Vcomp_native_path_postfix.  I could not test it but I suspect this is
not handled.

Thanks this is good to have in when tuned.

  Andrea

--
akrl@sdf.org

0001-Reduce-the-number-of-files-probed-when-finding-a-lis.patch
Description: Text Data