On Fri, 19 Feb 2021 16:53:21 +0100
Peter Krempa <pkrempa@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
