Performance improvement with 81f730d4

Performance improvement with 81f730d4

[Lukáš Doktor]

[-- Attachment #1.1.1: Type: text/plain, Size: 559 bytes --]

Hello Paolo,

the perf-ci detected and bisected the 81f730d4 - block, block-backend: write some hot coroutine wrappers by hand - as a performance improvement with 4K read/writes across various profiles. Without pinning it was about 10%, with pinned qemu about 15%.

    https://ldoktor.github.io/tmp/RedHat-virtlab722/v8.0.0/130-improvement.html

Based on the commit message I guess it's expected, so my question is whether I should report such obvious improvements or only report improvements/regressions that are not that obvious?

Regards,
Lukáš

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 12925 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: Performance improvement with 81f730d4

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

Hi Lukáš,

Am 19.04.2023 um 08:02 hat Lukáš Doktor geschrieben:
> Hello Paolo,
> 
> the perf-ci detected and bisected the 81f730d4 - block, block-backend:
> write some hot coroutine wrappers by hand - as a performance
> improvement with 4K read/writes across various profiles. Without
> pinning it was about 10%, with pinned qemu about 15%.
> 
>     https://ldoktor.github.io/tmp/RedHat-virtlab722/v8.0.0/130-improvement.html

I must admit that I don't really understand how to read this page, and
which build corresponds to which QEMU revision (or even which one is
older and which one is newer).

All the build names are links, but they only result in a 404.

> Based on the commit message I guess it's expected, so my question is
> whether I should report such obvious improvements or only report
> improvements/regressions that are not that obvious?

I think it's useful to know that we did indeed fix the performance
problem, so as far as I am concerned, please do report them.

When we're addressing a regression, the most interesting part would of
course be comparing to the version before the regression was introduced
(pre-d8fbf9aa85a in this case) to see if the regression was fully fixed.
Maybe the page you linked above does already explain this and I just
don't know where to look? :-)

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Performance improvement with 81f730d4

[Lukáš Doktor]

[-- Attachment #1.1.1: Type: text/plain, Size: 3676 bytes --]

Hello Kevin

Dne 19. 04. 23 v 10:16 Kevin Wolf napsal(a):
> Hi Lukáš,
> 
> Am 19.04.2023 um 08:02 hat Lukáš Doktor geschrieben:
>> Hello Paolo,
>>
>> the perf-ci detected and bisected the 81f730d4 - block, block-backend:
>> write some hot coroutine wrappers by hand - as a performance
>> improvement with 4K read/writes across various profiles. Without
>> pinning it was about 10%, with pinned qemu about 15%.
>>
>>     https://ldoktor.github.io/tmp/RedHat-virtlab722/v8.0.0/130-improvement.html
> 
> I must admit that I don't really understand how to read this page, and
> which build corresponds to which QEMU revision (or even which one is
> older and which one is newer).
> 
> All the build names are links, but they only result in a 404.

The report is originally designed for CI, where the links point to Jenkins results. In bisection I'm reusing the same report, replacing the links with full qemu commit hashes, which for some reason did not happen in this report. I'll take a look at that. The only way to tell the builds apart now is the 3-letter sha which is used as the build name. The builds are ordered as they appear in the git history.

You can see that 81f is the first one that performs differently. I'm using 2 out of 3 mode to improve reproducibility, which is why there are usually 2 builds of the same short name.

Usually I attach the bisect log but this time it was so obvious I skipped it. Let me attach it now to simplify mapping 3-letter short names to full qemu shas:

git bisect start
# good: [6c50845a9183610cfd4cfffd48dfc704cd340882] hw/i2c/allwinner-i2c: Fix subclassing of TYPE_AW_I2C_SUN6I
git bisect good 6c50845a9183610cfd4cfffd48dfc704cd340882
# bad: [7dbd6f8a27e30fe14adb3d5869097cddf24038d6] Update version for v8.0.0-rc4 release
git bisect bad 7dbd6f8a27e30fe14adb3d5869097cddf24038d6
# bad: [3fe64abcde55cf6f4ea5883106301baad219a7cc] block/nfs: do not poll within a coroutine
git bisect bad 3fe64abcde55cf6f4ea5883106301baad219a7cc
# good: [8c6f27e7d85a794698eb1cd32c58df28cece50d1] block: remove has_variable_length from BlockDriver
git bisect good 8c6f27e7d85a794698eb1cd32c58df28cece50d1
# good: [9ed98cae151368cc89c4bb77c9f325f7185e8f09] block-backend: ignore inserted state in blk_co_nb_sectors
git bisect good 9ed98cae151368cc89c4bb77c9f325f7185e8f09
# bad: [abb02ce0e76a8e00026699a863ab2d11d88f56d4] Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging
git bisect bad abb02ce0e76a8e00026699a863ab2d11d88f56d4
# bad: [81f730d4d0e8af9c0211c3fedf406df0046341a9] block, block-backend: write some hot coroutine wrappers by hand
git bisect bad 81f730d4d0e8af9c0211c3fedf406df0046341a9
# first bad commit: [81f730d4d0e8af9c0211c3fedf406df0046341a9] block, block-backend: write some hot coroutine wrappers by hand

> 
>> Based on the commit message I guess it's expected, so my question is
>> whether I should report such obvious improvements or only report
>> improvements/regressions that are not that obvious?
> 
> I think it's useful to know that we did indeed fix the performance
> problem, so as far as I am concerned, please do report them.
> 

OK, I'll keep doing that.

> When we're addressing a regression, the most interesting part would of
> course be comparing to the version before the regression was introduced
> (pre-d8fbf9aa85a in this case) to see if the regression was fully fixed.
> Maybe the page you linked above does already explain this and I just
> don't know where to look? :-)
> 

I wasn't aware of this to be addressing d8fbf9aa85a regression. If I find enough time I'll try to compare those.

Regards,
Lukáš

> Kevin

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 12925 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]