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...@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...@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 _______________________________________________ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg