Re: Slow down with: 'Make "info qom-tree" show children sorted'

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jul 07, 2020 at 06:45:57AM +0200, Thomas Huth wrote:
>> On 27/05/2020 10.47, Markus Armbruster wrote:
>> > "info qom-tree" prints children in unstable order.  This is a pain
>> > when diffing output for different versions to find change.  Print it
>> > sorted.
>> > 
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >  qom/qom-hmp-cmds.c | 24 ++++++++++++++++--------
>> >  1 file changed, 16 insertions(+), 8 deletions(-)
>> 
>>  Hi Markus,
>> 
>> this patch causes a slow down of the qtests which becomes quite massive
>> when e.g. using the ppc64 and thourough testing. When I'm running
>> 
>> QTEST_QEMU_BINARY="ppc64-softmmu/qemu-system-ppc64" time \
>> ./tests/qtest/device-introspect-test -m slow | tail -n 10
>> 
>> the test runs for ca. 6m40s here before the patch got applied, and for
>> mor than 20 minutes after the patch got applied!
>
> I think the test case itself could be optimized. Currently it does
> approx
>
>    for each device type
>       info qom-tree
>       device_addr type,help
>       info qom-tree
>
> it compares the before/after qom-tree to look for stray objects or
> to try to trigger crashes.
>
> The info qom-tree calls could be pushed outside the loop
>
>    info qom-tree
>    for each device type
>       device_addr type,help
>    info qom-tree
>
> Taking /x86_64/device/introspect/concrete/defaults/pc-q35-5.1 as a
> example, this change is the difference between 20 seconds running and
> 3 seconds running.

Patch?

> Reverting Markus' change actually didn't make much difference, only
> reducing the 20 seconds to 17 seconds.

I found and plugged a memory leak.  I haven't checked its impact on test
performance.

> The downside is that if there is a stray object/crash, it would not
> immediately associate with the device type. I'm not sure that's a
> real problem though. Especially if we are running this as pre-merge
> CI we'll only need to look at the patch series to find the broken
> device. If this is quick enough that we can run it as standard,
> instead of only with -m slow, then its a net win I think.

Easier investigation of failures is a valid argument, but not at this
price.  Our "make check" is painfully slow.