Re: [PATCH 3/8] migration/vmstate: avoid per-element heap churn in vmsd ptr marker field

[Fabiano Rosas <farosas@suse.de>]

Bin Guo <guobin@linux.alibaba.com> writes:

> For every NULL slot in a VMS_ARRAY_OF_POINTER (or every entry of a
> dynamic array), the saver allocates a 1-element fake VMStateField via
> g_new0 and frees it again right after the save. For arrays of
> thousands of entries this is thousands of malloc/free pairs on the
> hot save path.
>
> Replace the heap-allocated marker with a stack-resident field
> populated by an init helper. The caller passes a pointer to a local
> VMStateField, the helper fills it in (still asserting the
> precondition), and no g_free is needed.
>
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>  migration/vmstate.c | 41 ++++++++++++++++-------------------------
>  1 file changed, 16 insertions(+), 25 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 2f13b48a37..9ced78532f 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -59,29 +59,23 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
>   * array of a VMS_ARRAY_OF_POINTER VMSD field.  It's needed because we
>   * can't dereference the NULL pointer.
>   */
> -static const VMStateField *
> -vmsd_create_ptr_marker_field(const VMStateField *field)
> +static void
> +vmsd_init_ptr_marker_field(VMStateField *fake, const VMStateField *field)
>  {
> -    VMStateField *fake = g_new0(VMStateField, 1);
> -
>      /* It can only happen on an array of pointers! */
>      assert(field->flags & VMS_ARRAY_OF_POINTER);
>  
> -    /* Some of fake's properties should match the original's */
> -    fake->name = field->name;
> -    fake->version_id = field->version_id;
> -
> -    /* Do not need "field_exists" check as it always exists */
> -    fake->field_exists = NULL;
> -
> -    /* See vmstate_info_ptr_marker - use 1 byte to represent ptr status */
> -    fake->size = 1;
> -    fake->info = &vmstate_info_ptr_marker;
> -    fake->flags = VMS_SINGLE;
> -
> -    /* All the rest fields shouldn't matter.. */
> -
> -    return (const VMStateField *)fake;
> +    /* See vmstate_info_ptr_marker - 1 byte represents ptr status */
> +    *fake = (VMStateField) {
> +        .name = field->name,
> +        .version_id = field->version_id,
> +        /* Marker always exists, no field_exists callback needed */
> +        .field_exists = NULL,
> +        .size = 1,
> +        .info = &vmstate_info_ptr_marker,
> +        .flags = VMS_SINGLE,
> +        /* All other fields stay zero-initialised */
> +    };
>  }
>  
>  static int vmstate_n_elems(void *opaque, const VMStateField *field)
> @@ -680,6 +674,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>              for (i = 0; i < n_elems; i++) {
>                  void *curr_elem = first_elem + size * i;
>                  const VMStateField *inner_field;
> +                VMStateField marker_field;
>                  /* maximum number of elements to compress in the JSON blob */
>                  int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
>                  bool use_marker_field, is_null = false;
> @@ -693,7 +688,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  use_marker_field = use_dynamic_array || is_null;
>  
>                  if (use_marker_field) {
> -                    inner_field = vmsd_create_ptr_marker_field(field);
> +                    vmsd_init_ptr_marker_field(&marker_field, field);
> +                    inner_field = &marker_field;
>                  } else {
>                      inner_field = field;
>                  }
> @@ -730,11 +726,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>                                                      inner_field, vmdesc_loop,
>                                                      i, max_elems, errp);
>  
> -                /* If we used a fake temp field.. free it now */
> -                if (use_marker_field) {
> -                    g_clear_pointer((gpointer *)&inner_field, g_free);
> -                }
> -
>                  if (!ok) {
>                      goto out;
>                  }

Reviewed-by: Fabiano Rosas <farosas@suse.de>