[Back from enjoying the plague]
On Sun, Dec 8, 2013 at 4:53 PM, Branko Čibej <brane_at_wandisco.com> wrote:
> On 08.12.2013 11:52, Stefan Fuhrmann wrote:
>
> On Sun, Dec 8, 2013 at 11:27 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name>wrote:
>
>> Stefan Fuhrmann wrote on Sun, Dec 08, 2013 at 11:09:47 +0100:> >
>> >
>> > Duly noted for Subversion 2.0
>> >
>> > For now, however, we have to implement id_vtable_t.
>>
>> Huh? id_vtable_t is not a public API. I assume the id vtable falls
>> under the same rules as the fs_library vtable --- see
>> fs_library_vtable_t.get_version's docstring.
>>
>
> True, but the following API simply don't provide any more information:
>
> svn_fs_check_related(const svn_fs_id_t *a, const svn_fs_id_t *b)
> svn_fs_compare_ids(const svn_fs_id_t *a, const svn_fs_id_t *b)
>
> So, our private vtable depends on the data provided by public API.
> Adding the pool member to the internal struct is exactly the way to
> decouple the implementation from the existing public interface.
>
>
> There's nothing wrong with revising the public API to require a scratch
> pool. Sure, the deprecated, pool-less version will be less efficient. But
> I've yet to hear why "lean and smart", i.e., unstructured IDs are so much
> better. than what we have currently. After all, their structure matches the
> semantics of our repository model, regardless of implementation.
>
This is not about structured vs. unstructured IDs. It's about needlessly
exposing internal representation at the API level. For instance, it
requires
us to never change the internal node_id assignment rules (copy vs. branch):
svn_fs_compare_ids does not provide a pool argument; hence the ID struct
needs to always carry all information.
Despite the fact that it may take a few lookups to construct a single ID
(for each dirent and changed path passed back through the API), those
IDs are already huge. At 80 bytes each, they consume > 50% of the
memory in dirs and changed path lists. So, "lean" stands for "storing the
key only" and "smart" is for "gather derived information when actually
requested".
FSX will eventually (try to) implement your new node tree structure
explicitly modelling nodes, noderevs and copies / branches (semantics TBD)
and their keys / IDs might not be lumped together anymore. In a lazy copy
regime, not all node.copy.ver combinations may exist physically. Right now,
the API (in combination with the "no pools in structs" rule) prevents such
designs.
On Sun, Dec 8, 2013 at 5:05 PM, Branko Čibej <brane_at_wandisco.com> wrote:
> On 08.12.2013 16:34, Stefan Fuhrmann wrote:
>
> On Sun, Dec 8, 2013 at 1:31 PM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>> We revved almost the entire wc api for Subversion 1.7, with almost no
>> breakage for the older wc apis. (8 errata if I remember correctly). I don't
>> see why we can't do this for the fs or ra layer APIs before 2.0. The old
>> public APIs that miss arguments won't be as fast as the newer APIs that
>> have them but... I don't see this as an excuse to use a bad design for the
>> fs layer until 2.0.
>>
>
> Well, the alternative would be to eliminate svn_fs_id_t entirely from
> the API,
> which implies revving svn_fs_dirent_t and svn_fs_path_change2_t plus
> related functions. Simply introducing svn_fs_id2_t in parallel to
> svn_fs_id_t
> would needlessly duplicate a lot of code exactly because the underlying
> representations are incompatible.
>
> I would actually like to remve svn_fs_id_t from the API since it violates
> our general path_at_rev / path_at_root pattern (deprecating the old API for
> backward compat). Any objections?
>
> If we go down that route, we may as well begin designing the FSv2 API
> instead. This piecemeal twiddling for the sake of a back-end that we won't
> release for several years yet doesn't make much sense to me.
>
I'll stop exposing implementation details needlessly. The changed paths
APIs had been bumped in the past and bits of the ID API already got
deprecated a long time ago. Cleaning up the remnants will not create
FSv1.5 but a clearer FSv1.
See separate post.
On Sun, Dec 8, 2013 at 5:52 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name>wrote:
> Stefan Fuhrmann wrote on Sun, Dec 08, 2013 at 16:34:43 +0100:
> > I would actually like to remve svn_fs_id_t from the API since it violates
> > our general path_at_rev / path_at_root pattern (deprecating the old API for
> > backward compat). Any objections?
>
> Stefan, that's not a reason, that's discrimination. It doesn't actually
> explain why the functionality svn_fs_id_t provides is not needed by us
> or by API consumers.
>
How is discrimination in design matters a bad thing?
As for the why, see separate post.
-- Stefan^2.
Received on 2013-12-22 13:55:43 CET