| Message ID | 20241122084923.1542743-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,for-10.0] include/hw/boards: Optimize the booleans in MachineClass | expand |
On Fri, Nov 22, 2024 at 09:49:23AM +0100, Thomas Huth wrote: > While looking at the QEMU binary with "pahole", I noticed that we > could optimize the size of MachineClass a little bit: So far we > are using a mixture of a bitfield and single "bool" members here > for the boolean flags. Declaring all flags as part of the bitfield > helps to shrink the size of the struct a little bit. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > It's just a micro-optimization (the size of the struct decreases by > eight bytes), so I'm not sure whether it's worth the effort...? Given that this is a QOM class, rather than an instance, we'll only ever save memory once. That's an unmeasurably small real world improvement. We also have no ABI reasons to use bitfields for this. So if anything I'd suggest we take the opposite approach, and eliminate that bitfields in favour of just using 'bool' for everything, on the basis that a bitfield has no reason to exist. This could become useful if we aim to institute a general ban on the use of bitfields in QEMU, as a way to avoid the struct packing differences on Windows. > include/hw/boards.h | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 36fbb9b59d..c6946bd319 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -285,8 +285,16 @@ struct MachineClass { > no_cdrom:1, > no_sdcard:1, > pci_allow_0_address:1, > - legacy_fw_cfg_order:1; > - bool is_default; > + legacy_fw_cfg_order:1, > + is_default:1, > + auto_enable_numa_with_memhp:1, > + auto_enable_numa_with_memdev:1, > + ignore_boot_device_suffixes:1, > + smbus_no_migration_support:1, > + nvdimm_supported:1, > + numa_mem_supported:1, > + auto_enable_numa:1, > + cpu_cluster_has_numa_boundary:1; > const char *default_machine_opts; > const char *default_boot_order; > const char *default_display; > @@ -304,14 +312,6 @@ struct MachineClass { > int numa_mem_align_shift; > const char * const *valid_cpu_types; > strList *allowed_dynamic_sysbus_devices; > - bool auto_enable_numa_with_memhp; > - bool auto_enable_numa_with_memdev; > - bool ignore_boot_device_suffixes; > - bool smbus_no_migration_support; > - bool nvdimm_supported; > - bool numa_mem_supported; > - bool auto_enable_numa; > - bool cpu_cluster_has_numa_boundary; > SMPCompatProps smp_props; > const char *default_ram_id; > > -- > 2.47.0 > > With regards, Daniel
On 25/11/24 12:03, Daniel P. Berrangé wrote: > On Fri, Nov 22, 2024 at 09:49:23AM +0100, Thomas Huth wrote: >> While looking at the QEMU binary with "pahole", I noticed that we >> could optimize the size of MachineClass a little bit: So far we >> are using a mixture of a bitfield and single "bool" members here >> for the boolean flags. Declaring all flags as part of the bitfield >> helps to shrink the size of the struct a little bit. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> It's just a micro-optimization (the size of the struct decreases by >> eight bytes), so I'm not sure whether it's worth the effort...? > > Given that this is a QOM class, rather than an instance, we'll > only ever save memory once. That's an unmeasurably small real > world improvement. We also have no ABI reasons to use bitfields > for this. > > So if anything I'd suggest we take the opposite approach, and > eliminate that bitfields in favour of just using 'bool' for > everything, on the basis that a bitfield has no reason to > exist. While I agree with you on this, and the patch would take less than 1 minute, all of these fields are legacy options. Maybe a good opportunity to tackle this technical debt. I.e. as of 2024 having to set no_parallel/no_floppy/no_cdrom to true to every new machines seems counter intuitive. > This could become useful if we aim to institute a general ban > on the use of bitfields in QEMU, as a way to avoid the struct > packing differences on Windows. > >> include/hw/boards.h | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-)
On Mon, Nov 25, 2024 at 12:47:33PM +0100, Philippe Mathieu-Daudé wrote: > On 25/11/24 12:03, Daniel P. Berrangé wrote: > > On Fri, Nov 22, 2024 at 09:49:23AM +0100, Thomas Huth wrote: > > > While looking at the QEMU binary with "pahole", I noticed that we > > > could optimize the size of MachineClass a little bit: So far we > > > are using a mixture of a bitfield and single "bool" members here > > > for the boolean flags. Declaring all flags as part of the bitfield > > > helps to shrink the size of the struct a little bit. > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > --- > > > It's just a micro-optimization (the size of the struct decreases by > > > eight bytes), so I'm not sure whether it's worth the effort...? > > > > Given that this is a QOM class, rather than an instance, we'll > > only ever save memory once. That's an unmeasurably small real > > world improvement. We also have no ABI reasons to use bitfields > > for this. > > > > So if anything I'd suggest we take the opposite approach, and > > eliminate that bitfields in favour of just using 'bool' for > > everything, on the basis that a bitfield has no reason to > > exist. > > While I agree with you on this, and the patch would take less > than 1 minute, all of these fields are legacy options. Maybe > a good opportunity to tackle this technical debt. > > I.e. as of 2024 having to set no_parallel/no_floppy/no_cdrom > to true to every new machines seems counter intuitive. We don't set them for all machines though, they are all set selectively, and this is machine ABI controlled. So we could only remove this flags, once all versioned machines that have them active, are deleted. With regards, Daniel
diff --git a/include/hw/boards.h b/include/hw/boards.h index 36fbb9b59d..c6946bd319 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -285,8 +285,16 @@ struct MachineClass { no_cdrom:1, no_sdcard:1, pci_allow_0_address:1, - legacy_fw_cfg_order:1; - bool is_default; + legacy_fw_cfg_order:1, + is_default:1, + auto_enable_numa_with_memhp:1, + auto_enable_numa_with_memdev:1, + ignore_boot_device_suffixes:1, + smbus_no_migration_support:1, + nvdimm_supported:1, + numa_mem_supported:1, + auto_enable_numa:1, + cpu_cluster_has_numa_boundary:1; const char *default_machine_opts; const char *default_boot_order; const char *default_display; @@ -304,14 +312,6 @@ struct MachineClass { int numa_mem_align_shift; const char * const *valid_cpu_types; strList *allowed_dynamic_sysbus_devices; - bool auto_enable_numa_with_memhp; - bool auto_enable_numa_with_memdev; - bool ignore_boot_device_suffixes; - bool smbus_no_migration_support; - bool nvdimm_supported; - bool numa_mem_supported; - bool auto_enable_numa; - bool cpu_cluster_has_numa_boundary; SMPCompatProps smp_props; const char *default_ram_id;
While looking at the QEMU binary with "pahole", I noticed that we could optimize the size of MachineClass a little bit: So far we are using a mixture of a bitfield and single "bool" members here for the boolean flags. Declaring all flags as part of the bitfield helps to shrink the size of the struct a little bit. Signed-off-by: Thomas Huth <thuth@redhat.com> --- It's just a micro-optimization (the size of the struct decreases by eight bytes), so I'm not sure whether it's worth the effort...? include/hw/boards.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)