[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