[PATCH 21/21] qemuTestCapsCacheInsert: Rewrite caps cache insertion

[Ján Tomko]

On a Thursday in 2022, Peter Krempa wrote:
>Until now we did 2 weird things when inserting the qemuCaps used for
>individual test cases into the capability cache:
>
>1) we inserted the same caps for all emulators
>2) we always (expensively) copied them
>
>Now when real capabilities are used we don't touch them at all just
>simply inser them. This allows us one big optimization, by trading a

*insert

s/allows/gives/

>copy for just a virObjectRef as we can borrow the caps object to the
>cache.
>
>For fake caps we still copy them as we insert the fake machine types
>into them, but second big optimization is to insert the capabilities
>only for the architecture they belong to.
>
>Additionally this commit also ensures that all other entries in the
>cache for the binary are poisoned by empty caps so that it's obvious
>that the test is doing the right thing.
>
>Apart from this making actually more sense this shaves off more than 40%
>of runtime from qemuxml2argvtest.
>

Before:
Benchmark #1: ./run tests/qemuxml2argvtest
   Time (mean ± σ):      2.309 s ±  0.042 s    [User: 2.194 s, System: 0.098 s]
   Range (min … max):    2.231 s …  2.348 s    10 runs

After:
Benchmark #1: ./run tests/qemuxml2argvtest
   Time (mean ± σ):      1.321 s ±  0.024 s    [User: 1.209 s, System: 0.097 s]
   Range (min … max):    1.287 s …  1.346 s    10 runs

Thank you.

>Signed-off-by: Peter Krempa <pkrempa at redhat.com>
>---
> tests/testutilsqemu.c | 85 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 70 insertions(+), 15 deletions(-)
>
>diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
>index 94ff538382..cb665e501b 100644
>--- a/tests/testutilsqemu.c
>+++ b/tests/testutilsqemu.c
>@@ -385,30 +385,85 @@ qemuTestCapsPopulateFakeMachines(virQEMUCaps *caps,

[...]

>+    if (caps && virQEMUCapsGetArch(caps) != VIR_ARCH_NONE) {
>+        /* for capabilities which have architecture set we populate only the
>+         * given architecture and poison all other so that the test doesn't
>+         * accidentally test a weird combination */
>+        virArch arch = virQEMUCapsGetArch(caps);
>+        g_autoptr(virQEMUCaps) emptyCaps = virQEMUCapsNew();
>+        g_autoptr(virQEMUCaps) copyCaps = NULL;
>+        virQEMUCaps *effCaps = caps;
>

As-is, the scope of effCaps and copyCaps can be reduced.

>-        if (!tmpCaps)
>+        if (!emptyCaps)
>             return -1;
>
>-        if (!virQEMUCapsHasMachines(tmpCaps))
>-            qemuTestCapsPopulateFakeMachines(tmpCaps, i);
>+        if (arch_alias[arch] != VIR_ARCH_NONE)
>+            arch = arch_alias[arch];
>
>-        if (virFileCacheInsertData(cache, qemu_emulators[i], tmpCaps) < 0) {
>-            virObjectUnref(tmpCaps);
>-            return -1;

[...]

>+        for (i = 0; i < G_N_ELEMENTS(qemu_emulators); i++) {
>+            g_autoptr(virQEMUCaps) tmp = NULL;
>+
>+            if (qemu_emulators[i] == NULL)
>+                continue;
>+

>+            if (caps)
>+                tmp = virQEMUCapsNewCopy(caps);
>+            else
>+                tmp = virQEMUCapsNew();
>+
>+            if (!tmp)
>+                return -1;
>+
>+            qemuTestCapsPopulateFakeMachines(tmp, i);
>+

If you also split out this into something like
'qemuTestCapsCopyIfMachinesMissing', and fit all three cases into a
single for loop, the function will fit on my screen. Something like:

     for (i = 0; i < G_N_ELEMENTS(qemu_emulators); i++) {
         g_autoptr(virQEMUCaps) copyCaps = NULL;
         virQEMUCaps *insertCaps = caps;

         if (!qemu_emulators[i])
             continue;

         if (arch != VIR_ARCH_NONE && i != arch) {
             /* for capabilities which have architecture set we populate only the
              * given architecture and poison all other so that the test doesn't
              * accidentally test a weird combination */
             insertCaps = emptyCaps;
         } else {
             /* if we are dealing with fake caps we need to populate machine types */
             if (qemuTestCapsCopyIfMachinesMissing(&copyCaps, caps, i) < 0)
                 return -1;

             if (copyCaps)
                 insertCaps = copyCaps;
             else
                 insertCaps = caps;
         }

         if (qemuTestCapsCacheInsertData(cache, qemu_emulators[i], insertCaps) < 0)
             return -1;
     }


But I have not convinced myself yet whether it's more readable or not.

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220106/89ddd2fc/attachment-0001.sig>