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@xxxxxxxxxx> --- 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(©Caps, 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@xxxxxxxxxx>
Jano
Attachment:
signature.asc
Description: PGP signature
