[pve-devel] [PATCH qemu-server 1/1] config: only fetch necessary default values in get_derived_property helper

[Daniel Kral]

On Wed Oct 15, 2025 at 4:31 PM CEST, Fiona Ebner wrote:
> Am 30.09.25 um 4:20 PM schrieb Daniel Kral:
>> get_derived_property(...) is called in the semi-hot path of the HA
>> Manager's static load scheduler to retrieve the static stats of each VM.
>> As the defaults are only needed in certain cases and for a very small
>> subset of properties in the VM config, get those separately when needed.
>> 
>> Signed-off-by: Daniel Kral <d.kral at proxmox.com>
>> ---
>> get_current_memory(...) is still quite costly here, because it calls
>> parse_memory(...), which calls
>> PVE::JSONSchema::parse_property_string(...), which adds up for many
>> guest configurations parsed in every manage(...) call, but this already
>> helps quite a lot.
>
> If this really is a problem, we could do our own parsing, i.e. returning
> the value if the property string starts with \d+ or current=\d+ and
> falling back to get_current_memory() if it doesn't. Of course also using
> the default from $memory_fmt if not set.
>
>> 
>>  src/PVE/QemuConfig.pm | 8 +++-----
>>  src/PVE/QemuServer.pm | 6 ++++++
>>  2 files changed, 9 insertions(+), 5 deletions(-)
>> 
>> diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm
>> index d0844c4c..078c87e0 100644
>> --- a/src/PVE/QemuConfig.pm
>> +++ b/src/PVE/QemuConfig.pm
>> @@ -582,12 +582,10 @@ sub load_current_config {
>
> We could go a step further and save the three defaults we are interested
> in during module load into variables. Then you also save the hash
> accesses into $confdesc and $memory_fmt.
>
>>  sub get_derived_property {
>>      my ($class, $conf, $name) = @_;
>>  
>> -    my $defaults = PVE::QemuServer::load_defaults();
>> -
>>      if ($name eq 'max-cpu') {
>> -        my $cpus =
>> -            ($conf->{sockets} || $defaults->{sockets}) * ($conf->{cores} || $defaults->{cores});
>> -        return $conf->{vcpus} || $cpus;
>> +        my $sockets = $conf->{sockets} || PVE::QemuServer::get_default_property_value('sockets');
>> +        my $cores = $conf->{cores} || PVE::QemuServer::get_default_property_value('cores');
>> +        return $conf->{vcpus} || ($sockets * $cores);
>>      } elsif ($name eq 'max-memory') { # current usage maximum, not maximum hotpluggable
>>          return get_current_memory($conf->{memory}) * 1024 * 1024;
>>      } else {
>
> Question is, how much do we really need to optimize the function here?
> I'm not against it, but just want to note that looking at the static
> usage will always have its limitations in practice (independent of
> performance). With PSI-based usage, we should only need the static
> information for to-be-started or to-be-recovered guests rather than all,
> so performance of get_derived_property() becomes much less relevant. Or
> what do you think?

That's a good point, in that regard I definitely favor maintainability
over minor performance improvements here. The (one-time) profile of
9,000 HA resources being balanced on start with all patches applied
(except ha #9) shows the following runtimes:

+---------------------------------------------+------------+------------+
|                  Function                   | Excl. time | Incl. time |
+---------------------------------------------+------------+------------+
| PVE::HA::Resources::PVEVM::get_static_stats | 55 ms      | 437 ms     |
| PVE::QemuConfig::get_derived_property       | 40 ms      | 363 ms     |
| PVE::QemuServer::Memory::get_current_memory | 23 ms      | 340 ms     |
| PVE::QemuServer::Memory::parse_memory       | 22 ms      | 317 ms     |
| PVE::JSONSchema::parse_property_string      | 94 ms      | 223 ms     |
+---------------------------------------------+------------+------------+

parse_property_string does show up as the 6th highest-exclusive-runtime
function, but my main objective was that that `manage(...)` should
finish under ~10 seconds for ~10,000 HA resources. That should be doable
now, especially as the amount of calls to get_static_usage(...) is now
constant per manage(...) call instead of proportional to the amount of
HA resource state changes.

So let's stay with the current patch unless there are actually people
running into this..

By the way, the highest-exclusive-runtime function in that profile is
Sys::Syslog::syslog now (excluding CORE::sleep of course) ;). But since
that should happen only in very special cases, I don't think that's an
actual performance problem either.