Re: svn commit: r1723876 - in /subversion/branches/fs-node-api/subversion: include/svn_fs.h libsvn_fs/fs-loader.c libsvn_fs/fs-loader.h libsvn_fs/node_compat.c libsvn_fs_fs/node.c

Bert Huijben <bert_at_qqmail.nl> writes:

> The more interesting question is: what is the status of this branch?
>
> I would like to see this merged to trunk, unless there are some new problems.

(Sorry for the delay in response.)

I think that the new API makes sense by itself, and could be useful. But
we didn't see any noticeable performance improvement from switching to it.
Although the server does less work, the actual numbers are only less by a
few percent.

I plan to switch the last remaining callers in reporter.c to the new API, and
then we can decide on what to do next.

> I would say the relation is +-:
>
> Fs -> root -> node.
>
> In that case I would say the node should hold a pointer to root (which
> already holds a pointer to fs).
>
> Why add a direct pointer 2 levels up, when the step via root is more
> obvious and probably already available.
>
> (This is all about the member variable... I don't see a problem with the
> accessor function. It might not be needed later on after more cleanup as
> callers most likely already have all/most these pointers itself)

A node is detached from the particular root. The reason behind this is that
a root can be explicitly deallocated with svn_fs_close_root() (that's what
the reporter does when it keeps a fixed size LRU cache for open roots),
and this approach avoids having a node possibly referring to a deallocated
svn_fs_root_t.

As for the member variable, I think that we can replace it with a vtable
accessor, since we do have the information about svn_fs_t on lower layers.

Regards,
Evgeny Kotkov
Received on 2016-02-06 01:33:49 CET