On Tue, Aug 7, 2012 at 12:46 AM, Junio C Hamano <gits...@pobox.com> wrote:
> The index_ops abstraction is a right way to go, and I like it, but I
> think the split illustrated in this patch might turn out to be at
> wrong levels (and it is OK, as I understand this is a illustration
> of concept patch).
> For example, add_to_index() interface may be a good candidate to
> have in index_ops.  Because your in-core index may not be holding
> everything in a flat array, "find the location in the flat array the
> entry would sit, replace the existing one if there is any, otherwise
> insert" cannot be a generic way to add a new entry.  If you make the
> whole thing an abstract API entry point, a sparse implementation of
> the in-core index could still implement it without bringing the
> untouched and irrelevant parts of the index to core.
>         Side note: with a tree-like implementation of the in-core
>         index, "find the location the entry would sit", "get the
>         entry at the location", "insert the entry at the location",
>         could still be a set of good abstract API, though.  The
>         definition of _location_ may be quite different from "the
>         offset of the entry counting from the beginning of a flat
>         array", which is what index_name_pos() returns.
> The story is the same on the removal front.  The current
> remove_index_entry_at() interface is tied to the flat array
> implementation, so "remove the nth entry from the beginning" is an
> inappropriate interface for anything but such an implementation
> (unless we come up with an abstract notion of the "location" that is
> usable efficiently in a tree-like implementation, that is).

add_to_index and remove_index_entry_at seem good places for the cut.
But do we need to redefine the location? I think we need to sketch out
a long term plan first. In my mind it's like this:

 - for 3-5 years since v5 is released, we support v2 and v5 in
parallel. Other code can take advantage of v5, but it must neither
sacrifice v2 performance, compatibility nor maintainability
 - after that, we deprecate v2. v2 is automatically converted to v5 in
memory. v2 perf may suffer but at that point we don't care any more as
the majority of users should have been migrated to v5 (*)

If the long term plan is actually that, we will need to stick to flat
array implementation for forseeable future as moving from it most
likely impacts v2 performance. When v5 is used, it must maintain two
views, tree and list, at the same time. We can then postpone thinking
about the redefinition until v2 is deprecated and in-core moved to
tree view only.

This might not be the best way forward as v2 incompatible features
(like keeping empty directories in index, what else?) may never come
until v2 is deprecated.

(*) this is questionable though. Depending on the benchmarks, we may
want to support both v2 and v5 for indefinite time with v2 recommended
for small projects and v5 the rest. If it's so, yeah we need to think
of better API now.

> I wish that the development of this topic was done more in a
> top-down direction, instead of bottom-up, so that it identified the
> necessary access patterns to the in-core index early and come up
> with a good set of abstract API first, and then only after that is
> done, came up with in-core and on-disk format to support the
> necessary operations.

Yeah, which is why I asked to try out partial reading/writing early as
I'm a learn by example kind of guy. Speaking of which, now that we
have something substantial, what should be done before this may be
considered for 'next'?

I don't think we should wait until it reaches full potential (i.e.
significant perf gain from all major index-related commands). Apart
from patch preparation, more testing and benchmarking, should we wait
until we get new public API or just use current index API?

One API addition that I (if nobody else) will do soon is
read_index_partial(<pathspec>) and adapt as many read-only commands as
possible to it. (v2 just ignores the pathspec input and loads the
whole thing, so all commands must be aware the the loaded may be more
than what they asked). But this can wait until v5 gets in.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to