[PATCH] Cache xkbcomp output for fast start-up v2

Peter Hutterer peter.hutterer at who-t.net
Tue Mar 31 18:40:02 PDT 2009


On Mon, Mar 30, 2009 at 08:03:25PM +0800, Li, Yan wrote:
> This is the v2 patch for your review. Changes since v1:
> 1. gratuitous changes are all removed
> 2. xkbcomp's results are cached in /var/cache/xkb
> 
> I've tested this patch with 1.6.0 release on i586 Linux.
> 
> Welcome comments.
> 
> N.B. although cares are taken, I have no environment to test the
> Windows code path.
> 
> -- 
> Li, Yan

> From bd2cef32bafffb76ba1aa570f6a716cfe4b5ca15 Mon Sep 17 00:00:00 2001
> From: Yan Li <yan.i.li at intel.com>
> Date: Mon, 30 Mar 2009 19:32:27 +0800
> Subject: [PATCH] Cache xkbcomp output for fast start-up v2
> 
> xkbcomp outputs will be cached in files with hashed keymap as
> names. This saves boot time for around 1s on commodity netbooks.
> 
> Signed-off-by: Yan Li <yan.i.li at intel.com>
> ---
>  configure.ac        |    4 +-
>  xkb/README.compiled |    8 ++--
>  xkb/ddxLoad.c       |  124 ++++++++++++++++++++++++++++++++++++++++++++-------
>  xkb/xkbfmisc.c      |   18 ++++++-
>  4 files changed, 128 insertions(+), 26 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index f2718b8..d246f00 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -476,9 +476,9 @@ AC_ARG_WITH(default-font-path, AS_HELP_STRING([--with-default-font-path=PATH], [
>  AC_ARG_WITH(xkb-path,         AS_HELP_STRING([--with-xkb-path=PATH], [Path to XKB base dir (default: ${datadir}/X11/xkb)]),
>  				[ XKBPATH="$withval" ],
>  				[ XKBPATH="${datadir}/X11/xkb" ])
> -AC_ARG_WITH(xkb-output,       AS_HELP_STRING([--with-xkb-output=PATH], [Path to XKB output dir (default: ${datadir}/X11/xkb/compiled)]),
> +AC_ARG_WITH(xkb-output,       AS_HELP_STRING([--with-xkb-output=PATH], [Path to XKB output dir (default: /var/cache/xkb)]),
>  				[ XKBOUTPUT="$withval" ],
> -				[ XKBOUTPUT="compiled" ])
> +				[ XKBOUTPUT="/var/cache/xkb" ])

this should be ${localstatedir}/cache/xkb instead.
note that when you're doing this change, you also need to remove the part that
checks for XKM_OUTPUT_DIR to be an absolute directory (in configure.ac)

>  AC_ARG_WITH(serverconfig-path, AS_HELP_STRING([--with-serverconfig-path=PATH],
>  				   [Directory where ancillary server config files are installed (default: ${libdir}/xorg)]),
>  				[ SERVERCONFIG="$withval" ],
> diff --git a/xkb/README.compiled b/xkb/README.compiled
> index 71caa2f..a4a2ae0 100644
> --- a/xkb/README.compiled
> +++ b/xkb/README.compiled
> @@ -4,10 +4,10 @@ current keymap and/or any scratch keymaps used by clients.  The X server
>  or some other tool might destroy or replace the files in this directory,
>  so it is not a safe place to store compiled keymaps for long periods of
>  time.  The default keymap for any server is usually stored in:
> -     X<num>-default.xkm
> -where <num> is the display number of the server in question, which makes
> -it possible for several servers *on the same host* to share the same 
> -directory.
> +     server-<SHA1>.xkm
> +
> +where <SHA1> is the SHA1 hash of keymap source, so that compiled
> +keymap of different keymap sources are stored in different files.
>  
>  Unless the X server is modified, sharing this directory between servers on
>  different hosts could cause problems.
> diff --git a/xkb/ddxLoad.c b/xkb/ddxLoad.c
> index 4d5dfb6..4cd9800 100644
> --- a/xkb/ddxLoad.c
> +++ b/xkb/ddxLoad.c
> @@ -32,6 +32,12 @@ THE USE OR PERFORMANCE OF THIS SOFTWARE.
>  #include <xkb-config.h>
>  #endif
>  
> +#ifdef HAVE_SHA1_IN_LIBMD /* Use libmd for SHA1 */
> +# include <sha1.h>
> +#else /* Use OpenSSL's libcrypto */
> +# include <stddef.h>  /* buggy openssl/sha.h wants size_t */
> +# include <openssl/sha.h>
> +#endif
>  #include <stdio.h>
>  #include <ctype.h>
>  #define	NEED_EVENTS 1
> @@ -58,10 +64,10 @@ THE USE OR PERFORMANCE OF THIS SOFTWARE.
>  	 * Making the server write to a subdirectory of that directory
>  	 * requires some work in the general case (install procedure
>  	 * has to create links to /var or somesuch on many machines),
> -	 * so we just compile into /usr/tmp for now.
> +	 * so we just compile into /var/cache/xkb for now.
>  	 */
>  #ifndef XKM_OUTPUT_DIR
> -#define	XKM_OUTPUT_DIR	"compiled/"
> +#define	XKM_OUTPUT_DIR	"/var/cache/xkb"
>  #endif

this part can be removed now. XKM_OUTPUT_DIR is always defined through
configure.ac.
  
>  #define	PRE_ERROR_MSG "\"The XKEYBOARD keymap compiler (xkbcomp) reports:\""
> @@ -179,6 +185,45 @@ OutputDirectory(
>  }
>  
>  static Bool    	
> +Sha1Asc(char sha1Asc[SHA_DIGEST_LENGTH*2+1], const char * input)
> +{
> +    int i;
> +    unsigned char sha1[SHA_DIGEST_LENGTH];
> +
> +#ifdef HAVE_SHA1_IN_LIBMD /* Use libmd for SHA1 */
> +    SHA1_CTX ctx;
> +
> +    SHA1Init (&ctx);
> +    SHA1Update (&ctx, input, strlen(input));
> +    SHA1Final (sha1, &ctx);
> +#else /* Use OpenSSL's libcrypto */
> +    SHA_CTX ctx;
> +    int success;
> +
> +    success = SHA1_Init (&ctx);
> +    if (! success)
> +	return BadAlloc;
> +
> +    success = SHA1_Update (&ctx, input, strlen(input));
> +    if (! success)
> +	return BadAlloc;
> +
> +    success = SHA1_Final (sha1, &ctx);
> +    if (! success)
> +	return BadAlloc;
> +#endif
> +
> +    /* convert sha1 to sha1_asc */
> +    for(i=0; i<SHA_DIGEST_LENGTH; ++i) {
> +        sprintf(sha1Asc+i*2, "%02X", sha1[i]);
> +    }
> +
> +    return Success;
> +}
> +
> +/* call xkbcomp and compile XKB keymap, return xkm file name in
> +   nameRtrn */
> +static Bool
>  XkbDDXCompileKeymapByNames(	XkbDescPtr		xkb,
>  				XkbComponentNamesPtr	names,
>  				unsigned		want,
> @@ -187,7 +232,10 @@ XkbDDXCompileKeymapByNames(	XkbDescPtr		xkb,
>  				int			nameRtrnLen)
>  {
>      FILE *	out;
> -    char	*buf = NULL, keymap[PATH_MAX], xkm_output_dir[PATH_MAX];
> +    char *	buf = NULL, xkmfile[PATH_MAX], xkmOutputDir[PATH_MAX];

any particular reason why you renamed xkm_output_dir to xkmOutputDir?
it fits better with the rest of the variable naming, but it's not really
necessary.

> +    char *	canonicalXkmfileName;
> +    char	sha1Asc[SHA_DIGEST_LENGTH*2+1], xkbKeyMapBuf[1024];
> +    int	ret;
>  
>      const char	*emptystring = "";
>      const char	*xkbbasedirflag = emptystring;
> @@ -198,15 +246,57 @@ XkbDDXCompileKeymapByNames(	XkbDescPtr		xkb,
>      /* WIN32 has no popen. The input must be stored in a file which is
>         used as input for xkbcomp. xkbcomp does not read from stdin. */
>      char tmpname[PATH_MAX];
> -    const char *xkmfile = tmpname;
> +    const char *xkbfile = tmpname;
>  #else
> -    const char *xkmfile = "-";
> +    const char *xkbfile = "-";
>  #endif
>  
> -    snprintf(keymap, sizeof(keymap), "server-%s", display);
> +    /* Write keymap source (xkbfile) to memory buffer `xkbKeyMapBuf',
> +       of which SHA1 is generated and used as result xkm file name  */
> +    memset(xkbKeyMapBuf, 0, sizeof(xkbKeyMapBuf));
> +    out = fmemopen(xkbKeyMapBuf, sizeof(xkbKeyMapBuf), "w");
> +    if (NULL == out) {
> +        ErrorF("[xkb] open xkbKeyMapBuf for writting\n");

typo: "writing", not "writting".

> +        return False;
> +    }
> +    ret = XkbWriteXKBKeymapForNames(out, names, xkb, want, need);
> +    fclose(out);
> +    if (!ret) {
> +        ErrorF("[xkb] generating XKB Keymap, giving up compiling Keymap\n");
> +        return False;
> +    }
> +    DebugF("[xkb] computing SHA1 of keymap\n");
> +    if (Success == Sha1Asc(sha1Asc, xkbKeyMapBuf)) {
> +        snprintf(xkmfile, sizeof(xkmfile), "server-%s", sha1Asc);
> +    }
> +    else {
> +        ErrorF("[xkb] computing SHA1 of keymap, using display name instead\n");
> +        snprintf(xkmfile, sizeof(xkmfile), "server-%s", display);
> +    }
> +
> +    XkbEnsureSafeMapName(xkmfile);
> +    OutputDirectory(xkmOutputDir, sizeof(xkmOutputDir));
>  
> -    XkbEnsureSafeMapName(keymap);
> -    OutputDirectory(xkm_output_dir, sizeof(xkm_output_dir));
> +    /* set nameRtrn, fail if it's too small */
> +    if ((strlen(xkmfile)+1 > nameRtrnLen) && nameRtrn) {
> +        ErrorF("[xkb] nameRtrn too small to hold xkmfile name\n");
> +        return False;
> +    }
> +    strncpy(nameRtrn, xkmfile, nameRtrnLen);
> +
> +    /* if the xkb file already exists, reuse it */
> +    canonicalXkmfileName = Xprintf("%s/%s.xkm", xkmOutputDir, xkmfile);

> +    LogMessage(X_INFO, "[xkb] xkmfile %s ", canonicalXkmfileName);
> +    if (access(canonicalXkmfileName, R_OK) == 0) {
> +        /* yes, we can reuse old xkb file */
> +        LogMessage(X_INFO, "reused\n");
> +        xfree(canonicalXkmfileName);
> +        return True;
> +    }
> +    LogMessage(X_INFO, "is being compiled\n");
> +    xfree(canonicalXkmfileName);
> +
> +    /* continue to call xkbcomp to compile the keymap */
>  

This leaves an awkward log message. 
(II) [xkb] xkmfile /foo/server-...  (II) is being compiled
Just print it as one log message, and i'd replace [xkb] with xkb: which is
more coeherent with the rest of the log. the [xkb] format with square brackets
is more used for ErrorF.


>  #ifdef WIN32
>      strcpy(tmpname, Win32TempDir());
> @@ -235,9 +325,9 @@ XkbDDXCompileKeymapByNames(	XkbDescPtr		xkb,
>  		  xkbbindir, xkbbindirsep,
>  		  ( (xkbDebugFlags < 2) ? 1 :
>  		    ((xkbDebugFlags > 10) ? 10 : (int)xkbDebugFlags) ),
> -		  xkbbasedirflag, xkmfile,
> +		  xkbbasedirflag, xkbfile,
>  		  PRE_ERROR_MSG, ERROR_PREFIX, POST_ERROR_MSG1,
> -		  xkm_output_dir, keymap);
> +		  xkmOutputDir, xkmfile);
>  
>      if (xkbbasedirflag != emptystring) {
>  	xfree(xkbbasedirflag);
> @@ -256,7 +346,12 @@ XkbDDXCompileKeymapByNames(	XkbDescPtr		xkb,
>         XkbWriteXKBKeymapForNames(stderr,names,xkb,want,need);
>      }
>  #endif
> -	XkbWriteXKBKeymapForNames(out,names,xkb,want,need);
> +        /* write XKBKeyMapBuf to xkbcomp */
> +        if (EOF==fputs(xkbKeyMapBuf, out))
> +        {
> +            ErrorF("[xkb] sending keymap to xkbcomp\n");
> +            return False;
> +        }
>  #ifndef WIN32
>  	if (Pclose(out)==0)
>  #else
> @@ -265,16 +360,12 @@ XkbDDXCompileKeymapByNames(	XkbDescPtr		xkb,
>  	{
>              if (xkbDebugFlags)
>                  DebugF("[xkb] xkb executes: %s\n",buf);
> -	    if (nameRtrn) {
> -		strncpy(nameRtrn,keymap,nameRtrnLen);
> -		nameRtrn[nameRtrnLen-1]= '\0';
> -	    }
>              if (buf != NULL)
>                  xfree (buf);
>  	    return True;
>  	}
>  	else
> -	    LogMessage(X_ERROR, "Error compiling keymap (%s)\n", keymap);
> +            LogMessage(X_ERROR, "Error compiling keymap (%s)\n", xkbfile);

tab/spaces mixup

>  #ifdef WIN32
>          /* remove the temporary file */
>          unlink(tmpname);
> @@ -375,7 +466,6 @@ unsigned	missing;
>  	DebugF("Loaded XKB keymap %s, defined=0x%x\n",fileName,(*xkbRtrn)->defined);
>      }
>      fclose(file);
> -    (void) unlink (fileName);
>      return (need|want)&(~missing);
>  }
>  
> diff --git a/xkb/xkbfmisc.c b/xkb/xkbfmisc.c
> index ae752e9..5abf3c7 100644
> --- a/xkb/xkbfmisc.c
> +++ b/xkb/xkbfmisc.c
> @@ -293,15 +293,27 @@ unsigned	wantNames,wantConfig,wantDflts;
>      multi_section= 1;
>      if (((complete&XkmKeymapRequired)==XkmKeymapRequired)&&
>  	((complete&(~XkmKeymapLegal))==0)) {
> -	fprintf(file,"xkb_keymap \"%s\" {\n",name);
> +        if (fprintf(file,"xkb_keymap \"%s\" {\n",name) <= 0)
> +        {
> +            ErrorF("[xkb] writting XKB Keymap\n");
> +            return False;
> +        }
>      }
>      else if (((complete&XkmSemanticsRequired)==XkmSemanticsRequired)&&
>  	((complete&(~XkmSemanticsLegal))==0)) {
> -	fprintf(file,"xkb_semantics \"%s\" {\n",name);
> +        if (fprintf(file,"xkb_semantics \"%s\" {\n",name)<=0)
> +        {
> +            ErrorF("[xkb] writting XKB Keymap\n");
> +            return False;
> +        }
>      }
>      else if (((complete&XkmLayoutRequired)==XkmLayoutRequired)&&
>  	((complete&(~XkmLayoutLegal))==0)) {
> -	fprintf(file,"xkb_layout \"%s\" {\n",name);
> +        if (fprintf(file,"xkb_layout \"%s\" {\n",name)<=0)
> +        {
> +            ErrorF("[xkb] writting XKB Keymap\n");
> +            return False;
> +        }
>      }
>      else if (XkmSingleSection(complete&(~XkmVirtualModsMask))) {
>  	multi_section= 0;

shouldn't this hunk be in a separate patch? this part has changed in master
already anyway, so moving it into a separate patch would make it even easier
to port your patch to master (mind you, it takes less than 5 min now)

Cheers,
  Peter



More information about the xorg mailing list