On Wed, Nov 26, 2008 at 06:28:58AM -0800, Dan Nicholson wrote: > Rather than compiling a new keymap every time XkbInitKeyboardDeviceStruct is > called, cache the previous keymap and reuse it if the rules have not been > changed. When XkbSetRulesDflts is called, the cached map is invalidated if it > differs from the previous default rules. > > The map is also invalidated if named components are supplied. This could be > fixed by caching Ktcsg from the previous run and comparing. > > The meaning of xkb_cached_map has been changed in the code to reporesent the > keymap reused over multiple calls. The freshly compiled keymap used by > XkbInitDevice has been renamed to xkb_initial_map. > > Signed-off-by: Dan Nicholson <[EMAIL PROTECTED]> > --- > So, it turns out that XkbCopyKeymap doesn't quite do what you'd expect, > failing to copy a few fields of the XkbDescRec. I'm not sure whether it's > appropriate to copy .device_spec from the cached keymap, but it didn't > seem to hurt in my testing.
Tbh, I don't know either :) Daniel, any insights? I tried to reproduce the issue I pointed out with the last revision and it looks like the VCK doesn't switch keymaps properly anyway. That needs to be fixed, but it's not caused by your patch. > If this would be more appropriate in XkbCopyKeymap or maybe a new function > like XkbDuplicateKeymap, let me know. > > This patch also has a minor change to not mess with the XkbInitDevice > behavior from before. It might not be an issue, but it seems like having > it pick up any keymap other than one just setup in > XkbInitKeyboardDeviceStruct > would be wrong. > > xkb/xkbInit.c | 186 > ++++++++++++++++++++++++++++++++++++++------------------- > 1 files changed, 125 insertions(+), 61 deletions(-) > > diff --git a/xkb/xkbInit.c b/xkb/xkbInit.c > index ea28a5f..025978e 100644 > --- a/xkb/xkbInit.c > +++ b/xkb/xkbInit.c > @@ -130,6 +130,7 @@ static char * XkbLayoutUsed= NULL; > static char * XkbVariantUsed= NULL; > static char * XkbOptionsUsed= NULL; > > +static XkbDescPtr xkb_initial_map = NULL; > static XkbDescPtr xkb_cached_map = NULL; > > _X_EXPORT Bool noXkbExtension= XKB_DFLT_DISABLED; > @@ -244,31 +245,57 @@ _X_EXPORT void > XkbSetRulesDflts(char *rulesFile,char *model,char *layout, > char *variant,char *options) > { > - if (XkbRulesFile) > - _XkbFree(XkbRulesFile); > - XkbRulesFile= _XkbDupString(rulesFile); > - rulesDefined= True; > + Bool changed = False; > + > + if (!XkbRulesFile || strcmp(rulesFile, XkbRulesFile) != 0) { > + changed = True; > + if (XkbRulesFile) > + _XkbFree(XkbRulesFile); > + XkbRulesFile = _XkbDupString(rulesFile); > + } > + rulesDefined = True; > + > if (model) { > - if (XkbModelDflt) > - _XkbFree(XkbModelDflt); > - XkbModelDflt= _XkbDupString(model); > + if (!XkbModelDflt || strcmp(model, XkbModelDflt) != 0) { > + changed = True; > + if (XkbModelDflt) > + _XkbFree(XkbModelDflt); > + XkbModelDflt = _XkbDupString(model); > + } > } > + > if (layout) { > - if (XkbLayoutDflt) > - _XkbFree(XkbLayoutDflt); > - XkbLayoutDflt= _XkbDupString(layout); > + if (!XkbLayoutDflt || strcmp(layout, XkbLayoutDflt) != 0) { > + changed = True; > + if (XkbLayoutDflt) > + _XkbFree(XkbLayoutDflt); > + XkbLayoutDflt = _XkbDupString(layout); > + } > } > + > if (variant) { > - if (XkbVariantDflt) > - _XkbFree(XkbVariantDflt); > - XkbVariantDflt= _XkbDupString(variant); > + if (!XkbVariantDflt || strcmp(variant, XkbVariantDflt) != 0) { > + changed = True; > + if (XkbVariantDflt) > + _XkbFree(XkbVariantDflt); > + XkbVariantDflt = _XkbDupString(variant); > + } > } > + > if (options) { > - if (XkbOptionsDflt) > - _XkbFree(XkbOptionsDflt); > - XkbOptionsDflt= _XkbDupString(options); > + if (!XkbOptionsDflt || strcmp(options, XkbOptionsDflt) != 0) { > + changed = True; > + if (XkbOptionsDflt) > + _XkbFree(XkbOptionsDflt); > + XkbOptionsDflt = _XkbDupString(options); > + } > + } > + > + /* If the rules have changed, remove the cached map. */ > + if (changed && xkb_cached_map) { > + XkbFreeKeyboard(xkb_cached_map, XkbAllComponentsMask, True); > + xkb_cached_map = NULL; > } > - return; > } > > void > @@ -285,6 +312,8 @@ XkbDeleteRulesDflts() > _XkbFree(XkbOptionsDflt); > XkbOptionsDflt = NULL; > > + XkbFreeKeyboard(xkb_initial_map, XkbAllComponentsMask, True); > + xkb_initial_map = NULL; > XkbFreeKeyboard(xkb_cached_map, XkbAllComponentsMask, True); > xkb_cached_map = NULL; > } > @@ -488,14 +517,17 @@ XkbEventCauseRec cause; > if ( xkbi ) { > XkbDescPtr xkb; > > - if (xkb_cached_map) { > - xkbi->desc = xkb_cached_map; > - xkb_cached_map = NULL; > + xkbi->desc = XkbAllocKeyboard(); > + if (!xkbi->desc) > + FatalError("Couldn't allocate keyboard description\n"); > + if (xkb_initial_map) { > + if(!XkbCopyKeymap(xkb_initial_map, xkbi->desc, False)) > + FatalError("XKB: Couldn't copy initial keymap\n"); > + xkbi->desc->defined = xkb_initial_map->defined; > + xkbi->desc->flags = xkb_initial_map->flags; > + xkbi->desc->device_spec = xkb_initial_map->device_spec; > } > else { > - xkbi->desc= XkbAllocKeyboard(); > - if (!xkbi->desc) > - FatalError("Couldn't allocate keyboard description\n"); > xkbi->desc->min_key_code = pXDev->key->curKeySyms.minKeyCode; > xkbi->desc->max_key_code = pXDev->key->curKeySyms.maxKeyCode; > } > @@ -592,8 +624,6 @@ XkbDescPtr xkb; > return False; > pSyms= pSymsIn; > pMods= pModsIn; > - bzero(&defs,sizeof(XkbRF_VarDefsRec)); > - rules= XkbGetRulesDflts(&defs); > > /* > * The strings are duplicated because it is not guaranteed that > @@ -608,42 +638,76 @@ XkbDescPtr xkb; > if (names->geometry) names->geometry = _XkbDupString(names->geometry); > if (names->symbols) names->symbols = _XkbDupString(names->symbols); > > - if (defs.model && defs.layout && rules) { > - XkbComponentNamesRec rNames; > - bzero(&rNames,sizeof(XkbComponentNamesRec)); > - if (XkbDDXNamesFromRules(dev,rules,&defs,&rNames)) { > - if (rNames.keycodes) { > - if (!names->keycodes) > - names->keycodes = rNames.keycodes; > - else > - _XkbFree(rNames.keycodes); > - } > - if (rNames.types) { > - if (!names->types) > - names->types = rNames.types; > - else _XkbFree(rNames.types); > - } > - if (rNames.compat) { > - if (!names->compat) > - names->compat = rNames.compat; > - else _XkbFree(rNames.compat); > - } > - if (rNames.symbols) { > - if (!names->symbols) > - names->symbols = rNames.symbols; > - else _XkbFree(rNames.symbols); > - } > - if (rNames.geometry) { > - if (!names->geometry) > - names->geometry = rNames.geometry; > - else _XkbFree(rNames.geometry); > - } > - XkbSetRulesUsed(&defs); > - } > + /* > + * If named components were passed, we can't guarantee the validity > + * of the cached keymap. > + */ > + if (xkb_cached_map && > + (names->keymap || names->keycodes || names->types || > + names->compat || names->geometry || names->symbols)) { > + XkbFreeKeyboard(xkb_cached_map, XkbAllComponentsMask, True); > + xkb_cached_map = NULL; > } > > - ok = (Bool) XkbDDXLoadKeymapByNames(dev,names,XkmAllIndicesMask,0, > - &xkb,name,PATH_MAX); > + /* > + * If we have a cached compiled map, then the rules have not changed > + * and we can avoid recompiling. > + */ > + if (xkb_cached_map) { > + LogMessageVerb(X_INFO, 4, "XKB: Reusing cached keymap\n"); > + xkb = xkb_cached_map; > + ok = True; > + } > + else { > + bzero(&defs, sizeof(XkbRF_VarDefsRec)); > + rules = XkbGetRulesDflts(&defs); > + > + if (defs.model && defs.layout && rules) { > + XkbComponentNamesRec rNames; > + bzero(&rNames, sizeof(XkbComponentNamesRec)); > + > + if (XkbDDXNamesFromRules(dev, rules, &defs, &rNames)) { > + if (rNames.keycodes) { > + if (!names->keycodes) > + names->keycodes = rNames.keycodes; > + else > + _XkbFree(rNames.keycodes); > + } > + if (rNames.types) { > + if (!names->types) > + names->types = rNames.types; > + else > + _XkbFree(rNames.types); > + } > + if (rNames.compat) { > + if (!names->compat) > + names->compat = rNames.compat; > + else > + _XkbFree(rNames.compat); > + } > + if (rNames.symbols) { > + if (!names->symbols) > + names->symbols = rNames.symbols; > + else > + _XkbFree(rNames.symbols); > + } > + if (rNames.geometry) { > + if (!names->geometry) > + names->geometry = rNames.geometry; > + else > + _XkbFree(rNames.geometry); > + } > + XkbSetRulesUsed(&defs); > + } > + } > + > + ok = (Bool) XkbDDXLoadKeymapByNames(dev, names, XkmAllIndicesMask, > + 0, &xkb, name, PATH_MAX); > + > + /* Cache the map so it can be reused the next time. */ > + if (ok && xkb) > + xkb_cached_map = xkb; > + } > > if (ok && (xkb!=NULL)) { > KeyCode minKC,maxKC; > @@ -679,13 +743,13 @@ XkbDescPtr xkb; > } > /* Store the map here so we can pick it back up in XkbInitDevice. > * Sigh. */ > - xkb_cached_map = xkb; > + xkb_initial_map = xkb; > } > else { > LogMessage(X_WARNING, "Couldn't load XKB keymap, falling back to > pre-XKB keymap\n"); > } > ok= > InitKeyboardDeviceStruct((DevicePtr)dev,pSyms,pMods,bellProc,ctrlProc); > - xkb_cached_map = NULL; > + xkb_initial_map = NULL; > if ((pSyms==&tmpSyms)&&(pSyms->map!=NULL)) { > _XkbFree(pSyms->map); > pSyms->map= NULL; > -- > 1.5.6.5 Looks good to me. Cheers, Peter _______________________________________________ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg