[PATCH] Use cached XKB keymap when rules haven't changed
Peter Hutterer
peter.hutterer at who-t.net
Sun Nov 30 21:24:42 PST 2008
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 <dbn.lists at gmail.com>
> ---
> 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
More information about the xorg
mailing list