[libvirt] [PATCH 1/4] snapshot: framework for more efficient relation traversal

[Daniel Veillard]

On Mon, Oct 10, 2011 at 04:09:07PM -0600, Eric Blake wrote:
> On 10/09/2011 08:37 PM, Daniel Veillard wrote:
> >>This layout intentionally hardcodes the size of each snapshot,
> >>by tracking sibling pointers, rather than having to deal with
> >>the headache of yet more memory management by directly sticking
> >>a child[] on each parent.
> >
> >Okay, so you're using a 'first child'. But why try to maintain
> >'nchildren' since you can't get the list in 0(1) anyway and
> >need to walk the sibling. It's just redundant data to me, but maybe
> >I didn't understood the use :-)
> 
> See patch 3/4.  When no other flags are present,
> virDomainSnapshotNumChildren only needs to read the nchildren member
> [and virDomainSnapshotNum with LIST_ROOTS the nroots member] (for
> O(1) operation), rather than walk the list of first_child/sibling
> (for O(n) operation).  You are correct that
> virDomainSnapshotListChildrenNames has to walk the list, but since
> at least one API can benefit from not walking the list, we might as
> well track both pieces of information.

  Okay, agreed then, I was just wondering if more 'canonical' code
and data would lead to simpler code and hence easier to debug in case
of trouble, but we are already in the optimization phase, so fine :-)

> >>
> >>+
> >>+struct snapshot_set_relation {
> >>+    virDomainSnapshotObjListPtr snapshots;
> >>+    int err;
> >>+};
> >>+
> >
> >  The following function isn't trivial, worth adding a comment about
> >expected use.
> >
> >>+static void
> >>+virDomainSnapshotSetRelations(void *payload,
> >>+                              const void *name ATTRIBUTE_UNUSED,
> >>+                              void *data)
> 
> Indeed, although it is a callback function only used by
> virDomainSnapshotUpdateRelations.  Here's what I'll add:
> 
> /* Callback when iterating over a hash table of snapshots, where all
>  * snapshots start with no metadata.  For each snapshot with a parent,
>  * set the parent metadata field, and update the parent's list of
>  * children.  Set the error flag if a parent is not found or would
>  * cause a circular chain.  */
> 
> >
> >   Okay, just wondering about the usefulness of nchidren/nroot :-)
> 
> Did I manage to explain it?

  yes, certainly :-)

> >
> >   ACK
> 
> I'll wait on your feedback regarding whether I should attempt the
> meta-root concept now, or just push as-is and save a meta-root
> concept for later patches.

  Just push now and let see if the meta root patch actually lead to
better code.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/