Re: svn commit: r1506576 - in /subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs: fs.h fs_fs.c low_level.c temp_serializer.c transaction.c

[Stefan Fuhrmann <st...@wandisco.com>]

On Sun, Aug 4, 2013 at 9:08 AM, Daniel Shahaf <da...@elego.de> wrote:

> stefan2@apache.org wrote on Wed, Jul 24, 2013 at 14:32:48 -0000:
> > Author: stefan2
> > Date: Wed Jul 24 14:32:48 2013
> > New Revision: 1506576
> >
> > URL: http://svn.apache.org/r1506576
> > Log:
> > On the fsfs-improvements branch:  With the new ID types in place,
> > we can now replace other string with a struct
> >
> > +     intra-node uniqification content. */
> > +  struct
> > +    {
> > +      svn_fs_fs__id_part_t txn_id;
>
> I'd rename this inner-struct member to avoid confusion with the
> eponymous member of the outer struct.
>

Done in r1510134.


> As to the rest... I don't grok yet the new stuff.  Looks like there is
> an entirely new ID API, which was just thrown in with little docs as to
> the intended end-result or difference from the current one :-(
>

See log message of r1506545:

> This replaces the previous string-based API with one that represents
> IDs as quadruples of <revision,sub-id> pairs of integers.  Thus, it
> now matches the implicit usage of the node_id, branch_id, txn_id and
> rev_offset parts of those IDs.

The semantics of the previous ID API is being preserved.

Apart from bits improving symmetry (e.g. rev_offset is now
accessible as a whole just like the other parts), this is a
primarily syntactic change from string to struct. It's the
sheer amount of code churn makes it non-trivial.


> What is the difference between svn_fs_fs__id_txn_id()
> and svn_fs_fs__id_is_txn()?  Why can't one of them be deleted?
>

How do you delete an element from a struct? I could have
made svn_fs_fs__id_txn_id() return a NULL if the txn_id part
is not used but that would make the ID API less explicit:

if (svn_fs_fs__id_is_txn(id))
  do_stuff();

requires less knowledge about state machines etc. than

if (svn_fs_fs__id_txn_id(id) != NULL)
  do_stuff();


> svn_fs_fs__id_is_txn() uses svn_fs_fs__id_txn_used().  I haven't looked
> at callers, but wouldn't it be a coding error (i.e., an assert()-level
> bug) to have an fs_fs__id_t struct which has not been initialized to be
> either a revid or a txnid?


That would indeed be a coding *within* id.c. There is no API to
create an ID with both elements set nor to modify an existing ID.


>  Or do we have now a trimodal object
> [uninit'd, txnid, revid]?  (If the latter, I would wonder how many
> places in the code we have that assume a bimodal [txnid, revid]
> model...)
>

Hence the svn_fs_fs__id_is_txn() API. It returns a binary value.

Why does svn_fs_fs__id_is_txn() use a struct-extension instead of
> svn_fs_id_t.fsap_data?  Is that just in order to have svn_fs_id_t
> struct and the FSFS-private part of it be allocated contiguously?
>

Faster allocation, access and cache serialization.

Struct extensions ("inheritance") are one of the features of C++
I had loved to use here.

Why is svn_fs_fs__id_offset() not reimplemented as
>     #define svn_fs_fs__id_offset(id) svn_fs_fs__id_rev_offset(id).number
> ?  i.e., why are both accessors needed?
>

I don't see the added benefit of having a new macro calling a function
than having a new trivial function.

Having both accessors made it easier to migrate existing code.
We could now decide to get rid of the *_id_offset and *_id_rev API.
They are being called only in 2 and 4 places, respectively.


> Does the transition from 'const char *' to svn_fs_fs__id_part_t require
> changes to 'structure'?  The signature of svn_fs_fs__id_txn_create()
> implies that a "noderev id" is now a three-tuple of three (revnum,
> offset) pairs.  That doesn't match 'structure'.
>

'structure' is very explicit about the internal structure of
node_id, copy_id and txn_id. It also describes the internal
structure of the rev_offset element. ID API and implementation
now explicitly match that description instead of fiddling with
BDB-like IDs.

What is "rev node ID" in the docstring of svn_fs_fs__id_part_t?  Should
> it read "the txn-id component of a noderev-id"?
>

(Hopefully) clarified in r1510135.

Bottom line: I don't know that I understand this.  The duplication of
> svn_fs_fs__id_is_txn() svn_fs_fs__id_txn_used() makes me wary of NIH,
> and the other changes makes me worry if the all-too-common tendency to
> change the format without updating 'structure' has striken again.
>
> (Incidentally, trunk/subversion/libsvn_fs_x/structure is wrong --- it
> describes FSFS f6, not FSX.)
>

I am aware that the binary structure of FSX is not defined, yet.
I updated the file in r1510158.

-- Stefan^2.