[PATCH 4/4] qemu*xml2*test: Cache capabilities between tests

[Jonathon Jongsma]

On Fri, 19 Feb 2021 16:53:21 +0100
Peter Krempa <pkrempa at redhat.com> wrote:

> Invoking the XML parser every time is quite expensive. Since we have a
> deep copy function for 'virQEMUCapsPtr' object, we can cache the
> parsed results lazily.
> 
> This brings significant speedup to qemuxml2argvtest:
> 
> real	0m2.234s
> user	0m2.140s
> sys	0m0.089s
> 
> vs.
> 
> real	0m1.161s
> user	0m1.087s
> sys	0m0.072s
> 
> qemuxml2xmltest benefits too:
> 
> real	0m0.879s
> user	0m0.801s
> sys	0m0.071s
> 
> vs.
> 
> real	0m0.466s
> user	0m0.424s
> sys	0m0.040s
> 
> Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> ---
>  tests/qemustatusxml2xmltest.c |  3 ++-
>  tests/qemuxml2argvtest.c      |  3 ++-
>  tests/qemuxml2xmltest.c       |  3 ++-
>  tests/testutilsqemu.c         | 18 +++++++++++++++---
>  tests/testutilsqemu.h         |  1 +
>  5 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qemustatusxml2xmltest.c
> b/tests/qemustatusxml2xmltest.c index 67a070c986..2b2c409cba 100644
> --- a/tests/qemustatusxml2xmltest.c
> +++ b/tests/qemustatusxml2xmltest.c
> @@ -73,6 +73,7 @@ mymain(void)
>      g_autofree char *fakerootdir = NULL;
>      g_autoptr(virQEMUDriverConfig) cfg = NULL;
>      g_autoptr(GHashTable) capslatest = NULL;
> +    g_autoptr(GHashTable) capscache =
> virHashNew(virObjectFreeHashData); g_autoptr(virConnect) conn = NULL;
> 
>      capslatest = testQemuGetLatestCaps();
> @@ -109,7 +110,7 @@ mymain(void)
>          static struct testQemuInfo info = { \
>              .name = _name, \
>          }; \
> -        if (testQemuInfoSetArgs(&info, capslatest, \
> +        if (testQemuInfoSetArgs(&info, capscache, capslatest, \
>                                  ARG_QEMU_CAPS, QEMU_CAPS_LAST, \
>                                  ARG_END) < 0 || \
>              qemuTestCapsCacheInsert(driver.qemuCapsCache,
> info.qemuCaps) < 0) { \ diff --git a/tests/qemuxml2argvtest.c
> b/tests/qemuxml2argvtest.c index 2d538bee9c..d6d707cd24 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -789,6 +789,7 @@ mymain(void)
>      g_autofree char *fakerootdir = NULL;
>      g_autoptr(GHashTable) capslatest = NULL;
>      g_autoptr(GHashTable) qapiSchemaCache =
> virHashNew((GDestroyNotify) virHashFree);
> +    g_autoptr(GHashTable) capscache =
> virHashNew(virObjectFreeHashData);
> 
>      fakerootdir = g_strdup(FAKEROOTDIRTEMPLATE);
> 
> @@ -883,7 +884,7 @@ mymain(void)
>              .name = _name, \
>          }; \
>          info.qapiSchemaCache = qapiSchemaCache; \
> -        if (testQemuInfoSetArgs(&info, capslatest, \
> +        if (testQemuInfoSetArgs(&info, capscache, capslatest, \
>                                  __VA_ARGS__, ARG_END) < 0) \
>              return EXIT_FAILURE; \
>          testInfoSetPaths(&info, _suffix); \
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 5cd945f28f..01ac5886f2 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -85,6 +85,7 @@ mymain(void)
>      g_autofree char *fakerootdir = NULL;
>      g_autoptr(virQEMUDriverConfig) cfg = NULL;
>      g_autoptr(GHashTable) capslatest = NULL;
> +    g_autoptr(GHashTable) capscache =
> virHashNew(virObjectFreeHashData); g_autoptr(virConnect) conn = NULL;
> 
>      capslatest = testQemuGetLatestCaps();
> @@ -130,7 +131,7 @@ mymain(void)
>          static struct testQemuInfo info = { \
>              .name = _name, \
>          }; \
> -        if (testQemuInfoSetArgs(&info, capslatest, \
> +        if (testQemuInfoSetArgs(&info, capscache, capslatest, \
>                                  __VA_ARGS__, \
>                                  ARG_END) < 0 || \
>              qemuTestCapsCacheInsert(driver.qemuCapsCache,
> info.qemuCaps) < 0) { \ diff --git a/tests/testutilsqemu.c
> b/tests/testutilsqemu.c index a96c9d487a..1dcf5caf6a 100644
> --- a/tests/testutilsqemu.c
> +++ b/tests/testutilsqemu.c
> @@ -681,6 +681,7 @@ testQemuCapsIterate(const char *suffix,
> 
>  int
>  testQemuInfoSetArgs(struct testQemuInfo *info,
> +                    GHashTable *capscache,
>                      GHashTable *capslatest, ...)
>  {
>      va_list argptr;
> @@ -773,6 +774,7 @@ testQemuInfoSetArgs(struct testQemuInfo *info,
> 
>      if (!qemuCaps && capsarch && capsver) {
>          bool stripmachinealiases = false;
> +        virQEMUCapsPtr cachecaps = NULL;
> 
>          info->arch = virArchFromString(capsarch);
> 
> @@ -784,11 +786,21 @@ testQemuInfoSetArgs(struct testQemuInfo *info,
>                                         TEST_QEMU_CAPS_PATH, capsver,
> capsarch); }
> 
> -        if (!(qemuCaps = qemuTestParseCapabilitiesArch(info->arch,
> capsfile)))
> +        if (!g_hash_table_lookup_extended(capscache, capsfile, NULL,
> (void **) &cachecaps)) {
> +            if (!(qemuCaps =
> qemuTestParseCapabilitiesArch(info->arch, capsfile)))
> +                goto cleanup;
> +
> +            if (stripmachinealiases)
> +                virQEMUCapsStripMachineAliases(qemuCaps);
> +
> +            cachecaps = qemuCaps;
> +
> +            g_hash_table_insert(capscache, g_strdup(capsfile),
> g_steal_pointer(&qemuCaps));
> +        }
> +
> +        if (!(qemuCaps = virQEMUCapsNewCopy(cachecaps)))
>              goto cleanup;
> 
> -        if (stripmachinealiases)
> -            virQEMUCapsStripMachineAliases(qemuCaps);
>          info->flags |= FLAG_REAL_CAPS;
> 
>          /* provide path to the replies file for schema testing */
> diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h
> index 86ba69e96b..e1b386f234 100644
> --- a/tests/testutilsqemu.h
> +++ b/tests/testutilsqemu.h
> @@ -110,6 +110,7 @@ int testQemuCapsIterate(const char *suffix,
>                          void *opaque);
> 
>  int testQemuInfoSetArgs(struct testQemuInfo *info,
> +                        GHashTable *capscache,
>                          GHashTable *capslatest, ...);
>  void testQemuInfoClear(struct testQemuInfo *info);
> 


Series looks fine to me. One thing I find a little bit confusing in
this patch is that you have both 'cachecaps' and 'capscache' in the same
function. I think just renaming 'cachecaps' to 'caps' (or even
'cachedcaps' with a d) would make things slightly less confusing.

Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>