Don't scan and parse useless maps

One physical xkb file may (and usually does) contain multiple maps. For
example, the us symbols file contains a map for every variant.
Currently, when we need a map from a file (specific or default), we
parse the entire file into a list of XkbFile's, find the map we want and
discard the others. This happens for every include statement. This is a lot
of unnecessary work; this commit is a first step at making it better.

What we do now is make yyparse return one map at a time; if we find what
we want, we can stop looking and avoid processing the rest of the file.

This moves some logic from include.c to parser.y (i.e. finding the
correct map, named or default). It also necessarily removes the
CheckDefaultMap check, which warned about a file which contains multiple
default maps. We can live without it.

Some stats with test/rulecomp (under valgrind and the benchmark):

Before:
==2280==   total heap usage: 288,665 allocs, 288,665 frees, 13,121,349 bytes allocated
compiled 1000 keymaps in 10.849487353s

After:
==1070==   total heap usage: 100,197 allocs, 100,197 frees, 9,329,900 bytes allocated
compiled 1000 keymaps in 5.258960549s

Pretty good.

Note: we still do some unnecessary work, by parsing and discarding the
maps before the one we want. However dealing with this is more
complicated (maybe using bison's push-parser and sniffing the token
stream). Probably not worth it.

Signed-off-by: Ran Benita <ran234@gmail.com>

diff --git a/src/xkbcomp/include.c b/src/xkbcomp/include.c
index c5f8f07..b94ffd0 100644 (file)
--- a/src/xkbcomp/include.c
+++ b/src/xkbcomp/include.c
@@ -285,69 +285,37 @@ ProcessIncludeFile(struct xkb_context *ctx,
                    XkbFile ** file_rtrn, enum merge_mode *merge_rtrn)
 {
     FILE *file;
-    XkbFile *rtrn, *mapToUse, *next;
+    XkbFile *xkb_file;
 
     file = FindFileInXkbPath(ctx, stmt->file, file_type, NULL);
     if (!file)
         return false;
 
-    rtrn = XkbParseFile(ctx, file, stmt->file);
-    if (!rtrn) {
-        log_err(ctx, "Error interpreting include file \"%s\"\n", stmt->file);
+    xkb_file = XkbParseFile(ctx, file, stmt->file, stmt->map);
+    if (!xkb_file) {
+        if (stmt->map)
+            log_err(ctx, "Couldn't process include statement for '%s(%s)'\n",
+                    stmt->file, stmt->map);
+        else
+            log_err(ctx, "Couldn't process include statement for '%s'\n",
+                    stmt->file);
         fclose(file);
         return false;
     }
     fclose(file);
 
-    mapToUse = rtrn;
-    if (stmt->map != NULL) {
-        while (mapToUse)
-        {
-            next = (XkbFile *) mapToUse->common.next;
-            mapToUse->common.next = NULL;
-            if (streq(mapToUse->name, stmt->map) &&
-                mapToUse->file_type == file_type) {
-                FreeXkbFile(next);
-                break;
-            }
-            else {
-                FreeXkbFile(mapToUse);
-            }
-            mapToUse = next;
-        }
-        if (!mapToUse) {
-            log_err(ctx, "No %s named \"%s\" in the include file \"%s\"\n",
-                    xkb_file_type_to_string(file_type), stmt->map,
-                    stmt->file);
-            return false;
-        }
-    }
-    else if (rtrn->common.next) {
-        for (; mapToUse; mapToUse = (XkbFile *) mapToUse->common.next)
-            if (mapToUse->flags & MAP_IS_DEFAULT)
-                break;
-
-        if (!mapToUse) {
-            mapToUse = rtrn;
-            log_vrb(ctx, 5,
-                    "No map in include statement, but \"%s\" contains several; "
-                    "Using first defined map, \"%s\"\n",
-                    stmt->file, rtrn->name);
-        }
-    }
-
-    if (mapToUse->file_type != file_type) {
+    if (xkb_file->file_type != file_type) {
         log_err(ctx,
                 "Include file wrong type (expected %s, got %s); "
                 "Include file \"%s\" ignored\n",
                 xkb_file_type_to_string(file_type),
-                xkb_file_type_to_string(mapToUse->file_type), stmt->file);
+                xkb_file_type_to_string(xkb_file->file_type), stmt->file);
         return false;
     }
 
     /* FIXME: we have to check recursive includes here (or somewhere) */
 
-    *file_rtrn = mapToUse;
+    *file_rtrn = xkb_file;
     *merge_rtrn = stmt->merge;
     return true;
 }

diff --git a/src/xkbcomp/parser-priv.h b/src/xkbcomp/parser-priv.h
index 9069415..5490d16 100644 (file)
--- a/src/xkbcomp/parser-priv.h
+++ b/src/xkbcomp/parser-priv.h
@@ -27,15 +27,8 @@
 #ifndef XKBCOMP_PARSER_PRIV_H
 #define XKBCOMP_PARSER_PRIV_H
 
-#pragma GCC diagnostic ignored "-Wredundant-decls"
-
 struct scanner_extra;
-
-struct parser_param {
-    struct xkb_context *ctx;
-    void *scanner;
-    XkbFile *rtrn;
-};
+struct parser_param;
 
 #include "parser.h"
 
@@ -45,7 +38,7 @@ scanner_error(YYLTYPE *loc, void *scanner, const char *msg);
 int
 _xkbcommon_lex(YYSTYPE *val, YYLTYPE *loc, void *scanner);
 
-int
-_xkbcommon_parse(struct parser_param *param);
+XkbFile *
+parse(struct xkb_context *ctx, void *scanner, const char *map);
 
 #endif

diff --git a/src/xkbcomp/parser.y b/src/xkbcomp/parser.y
index 491e96c..28c107b 100644 (file)
--- a/src/xkbcomp/parser.y
+++ b/src/xkbcomp/parser.y
@@ -29,6 +29,13 @@
 #include "ast-build.h"
 #include "parser-priv.h"
 
+struct parser_param {
+    struct xkb_context *ctx;
+    void *scanner;
+    XkbFile *rtrn;
+    bool more_maps;
+};
+
 static void
 _xkbcommon_error(struct YYLTYPE *loc, struct parser_param *param, const char *msg)
 {
@@ -176,10 +183,24 @@ _xkbcommon_error(struct YYLTYPE *loc, struct parser_param *param, const char *ms
 
 %%
 
+/*
+ * An actual file may contain more than one map. However, if we do things
+ * in the normal yacc way, i.e. aggregate all of the maps into a list and
+ * let the caller find the map it wants, we end up scanning and parsing a
+ * lot of unneeded maps (in the end we always just need one).
+ * Instead of doing that, we make yyparse return one map at a time, and
+ * then call it repeatedly until we find the map we need. Once we find it,
+ * we don't need to parse everything that follows in the file.
+ * This does mean that if we e.g. always use the first map, the file may
+ * contain complete garbage after that. But it's worth it.
+ */
+
 XkbFile         :       XkbCompositeMap
-                        { $$ = param->rtrn = $1; }
-                |       XkbMapConfigList
-                        { $$ = param->rtrn = $1; }
+                        { $$ = param->rtrn = $1; param->more_maps = true; }
+                |       XkbMapConfig
+                        { $$ = param->rtrn = $1; param->more_maps = true; YYACCEPT; }
+                |       END_OF_FILE
+                        { $$ = param->rtrn = NULL; param->more_maps = false; }
                 ;
 
 XkbCompositeMap :       OptFlags XkbCompositeType OptMapName OBRACE
@@ -732,3 +753,50 @@ MapName         :       STRING  { $$ = $1; }
 %%
 
 #undef scanner
+
+XkbFile *
+parse(struct xkb_context *ctx, void *scanner, const char *map)
+{
+    struct parser_param param;
+    int ret;
+    XkbFile *first = NULL;
+
+    param.scanner = scanner;
+    param.ctx = ctx;
+
+    /*
+     * If we got a specific map, we look for it exclusively and return
+     * immediately upon finding it. Otherwise, we need to get the
+     * default map. If we find a map marked as default, we return it
+     * immediately. If there are no maps marked as default, we return
+     * the first map in the file.
+     */
+
+    while ((ret = yyparse(&param)) == 0 && param.more_maps) {
+        if (map) {
+            if (streq_not_null(map, param.rtrn->name))
+                return param.rtrn;
+            else
+                FreeXkbFile(param.rtrn);
+        }
+        else {
+            if (param.rtrn->flags & MAP_IS_DEFAULT) {
+                FreeXkbFile(first);
+                return param.rtrn;
+            }
+            else if (!first) {
+                first = param.rtrn;
+            }
+            else {
+                FreeXkbFile(param.rtrn);
+            }
+        }
+    }
+
+    if (ret != 0) {
+        FreeXkbFile(first);
+        return NULL;
+    }
+
+    return first;
+}

diff --git a/src/xkbcomp/scanner.l b/src/xkbcomp/scanner.l
index 570e0bd..5ccf3e9 100644 (file)
--- a/src/xkbcomp/scanner.l
+++ b/src/xkbcomp/scanner.l
@@ -29,6 +29,8 @@
 #include "parser-priv.h"
 
 #pragma GCC diagnostic ignored "-Wmissing-noreturn"
+#pragma GCC diagnostic ignored "-Wredundant-decls"
+#pragma GCC diagnostic push
 
 struct scanner_extra {
     struct xkb_context *ctx;
@@ -204,6 +206,8 @@ alternate_group         return ALTERNATE_GROUP;
 
 %%
 
+#pragma GCC diagnostic pop
+
 static void
 scanner_error_extra(struct YYLTYPE *loc, struct scanner_extra *extra,
                     const char *msg)
@@ -220,46 +224,6 @@ scanner_error(struct YYLTYPE *loc, void *scanner, const char *msg)
     scanner_error_extra(loc, extra, msg);
 }
 
-static void
-CheckDefaultMap(struct xkb_context *ctx, XkbFile *maps, const char *fileName)
-{
-    XkbFile *dflt = NULL, *tmp;
-
-    for (tmp = maps; tmp; tmp = (XkbFile *) tmp->common.next) {
-        if (!(tmp->flags & MAP_IS_DEFAULT))
-            continue;
-
-        if (!dflt) {
-            dflt = tmp;
-            continue;
-        }
-
-        log_vrb(ctx, 3,
-                "Multiple default components in %s; "
-                "Using %s, ignoring %s\n",
-                (fileName ? fileName : "(unknown)"),
-                (dflt->name ? dflt->name : "(first)"),
-                (tmp->name ? tmp->name : "(subsequent)"));
-
-        tmp->flags &= (~MAP_IS_DEFAULT);
-    }
-}
-
-static XkbFile *
-parse(struct xkb_context *ctx, yyscan_t scanner)
-{
-    struct parser_param param;
-
-    param.scanner = scanner;
-    param.ctx = ctx;
-
-    if (_xkbcommon_parse(&param) != 0)
-        return NULL;
-
-    CheckDefaultMap(param.ctx, param.rtrn, yyget_extra(scanner)->scanFile);
-    return param.rtrn;
-}
-
 static bool
 init_scanner(yyscan_t *scanner, struct scanner_extra *extra,
              struct xkb_context *ctx, const char *file_name)
@@ -295,7 +259,7 @@ XkbParseString(struct xkb_context *ctx, const char *string,
 
     state = yy_scan_string(string, scanner);
 
-    xkb_file = parse(ctx, scanner);
+    xkb_file = parse(ctx, scanner, NULL);
 
     yy_delete_buffer(state, scanner);
     clear_scanner(scanner);
@@ -304,7 +268,8 @@ XkbParseString(struct xkb_context *ctx, const char *string,
 }
 
 XkbFile *
-XkbParseFile(struct xkb_context *ctx, FILE *file, const char *file_name)
+XkbParseFile(struct xkb_context *ctx, FILE *file,
+             const char *file_name, const char *map)
 {
     yyscan_t scanner;
     struct scanner_extra extra;
@@ -317,7 +282,7 @@ XkbParseFile(struct xkb_context *ctx, FILE *file, const char *file_name)
     state = yy_create_buffer(file, YY_BUF_SIZE, scanner);
     yy_switch_to_buffer(state, scanner);
 
-    xkb_file = parse(ctx, scanner);
+    xkb_file = parse(ctx, scanner, map);
 
     yy_delete_buffer(state, scanner);
     clear_scanner(scanner);

diff --git a/src/xkbcomp/xkbcomp-priv.h b/src/xkbcomp/xkbcomp-priv.h
index f94cbf1..b2b40fd 100644 (file)
--- a/src/xkbcomp/xkbcomp-priv.h
+++ b/src/xkbcomp/xkbcomp-priv.h
@@ -38,7 +38,8 @@ struct xkb_component_names {
 };
 
 XkbFile *
-XkbParseFile(struct xkb_context *ctx, FILE *file, const char *file_name);
+XkbParseFile(struct xkb_context *ctx, FILE *file,
+             const char *file_name, const char *map);
 
 XkbFile *
 XkbParseString(struct xkb_context *ctx, const char *string,

diff --git a/src/xkbcomp/xkbcomp.c b/src/xkbcomp/xkbcomp.c
index 854a1e7..5025dc6 100644 (file)
--- a/src/xkbcomp/xkbcomp.c
+++ b/src/xkbcomp/xkbcomp.c
@@ -167,7 +167,7 @@ xkb_keymap_new_from_file(struct xkb_context *ctx,
         return NULL;
     }
 
-    xkb_file = XkbParseFile(ctx, file, "(unknown file)");
+    xkb_file = XkbParseFile(ctx, file, "(unknown file)", NULL);
     if (!xkb_file) {
         log_err(ctx, "Failed to parse input xkb file\n");
         return NULL;