[Fontconfig] [PATCH] Cache most recent regcomp() call in _FcStrRegexCmp().

[Behdad Esfahbod]

On 13-05-01 05:29 PM, Nick Alcock wrote:
> 
> Under the circumstances, it is unfortunate that _FcStrRegexCmp() is called so
> very often by fontconfig.  On my system (with ~5000 fonts, and a stripped-down
> fonts.conf), mapping a new Emacs frame makes twelve calls to FcFontMatch()...
> and those twelve calls proceed to call _FcStrRegexCmp() 175171 times, with a
> call to regcomp() every time!  This then proceeds to invoke malloc() on the
> order of three million times.  It is fairly easy to turn this into a
> pathological slowdown at runtime.  My Emacsen are not small (arena sizes of a
> gigabyte-plus are common, with a highly fragmented heap), and in that situation
> those three million malloc() and free() calls can easily consume in excess of
> fifty seconds (though on a fresh startup it takes 'only' a quarter of a second
> or so).

It's really strange that that function is called at all.  Can you figure out
why that is?  Does your configuration have anything involving the "file" element?

Akira, regardless, I think we should remove the Regex and replace it with Glob
matching that is already in fccfg.c.

I know you want to extend regex to other elements, but for files I think globs
are just fine.

behdad

> Since each call to FcFontMatch() in this pathological case (and probably in most
> other cases) calls _FcStrRegexCmp() with a constant argument, a trivial cache
> consisting of a single static variable suffices to reduce the number of calls to
> regcomp() to... eleven.  (The precise method used may need a little rethinking
> for thread-safety, but the general principle, of reducing regcomp() calls via
> some variety of caching, appears to be essential.)
> 
> ---
>  src/fcstr.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> [Resent for the second time: if this doesn't get through I will consider
>  the fontconfig list broken...]
> 
> diff --git a/src/fcstr.c b/src/fcstr.c
> index 339a346..fa4560d 100644
> --- a/src/fcstr.c
> +++ b/src/fcstr.c
> @@ -247,19 +247,32 @@ static FcBool
>  _FcStrRegexCmp (const FcChar8 *s, const FcChar8 *regex, int cflags, int eflags)
>  {
>      int ret = -1;
> -    regex_t reg;
> +    static regex_t reg;
> +    static FcChar8 *last;
>  
> -    if ((ret = regcomp (&reg, (const char *)regex, cflags)) != 0)
> +    if (!regex || !last || FcStrCmp (regex, last) != 0)
>      {
> -	if (FcDebug () & FC_DBG_MATCHV)
> +	if (last != NULL)
>  	{
> -	    char buf[512];
> +	    regfree (&reg);
> +	    last = NULL;
> +	}
>  
> -	    regerror (ret, &reg, buf, 512);
> -	    printf("Regexp compile error: %s\n", buf);
> +	if ((ret = regcomp (&reg, (const char *)regex, cflags)) != 0)
> +	{
> +	    if (FcDebug () & FC_DBG_MATCHV)
> +	    {
> +		char buf[512];
> +
> +		regerror (ret, &reg, buf, 512);
> +		printf("Regexp compile error: %s\n", buf);
> +	    }
> +	    return FcFalse;
>  	}
> -	return FcFalse;
> +	FcFree (last);
> +	last = FcStrdup (regex);
>      }
> +
>      ret = regexec (&reg, (const char *)s, 0, NULL, eflags);
>      if (ret != 0)
>      {
> @@ -271,7 +284,9 @@ _FcStrRegexCmp (const FcChar8 *s, const FcChar8 *regex, int cflags, int eflags)
>  	    printf("Regexp exec error: %s\n", buf);
>  	}
>      }
> -    regfree (&reg);
> +
> +    if (last == NULL)  /* only on OOM */
> +	regfree (&reg);
>  
>      return ret == 0 ? FcTrue : FcFalse;
>  }
> 

-- 
behdad
http://behdad.org/