Re: [PATCH 2/4] pack v4: add v4_size to struct delta_base_cache_entry

From: Duy Nguyen <pclouds@gmail.com>
Date: Sep 13, 2013, 13:59 UTC
Message-ID: <CACsJy8DrxQqvKyAH0trd1nDbFTL2Gq7k5O-shK4j8W6PMirNYw@mail.gmail.com>
In-Reply-To: <alpine.LFD.2.03.1309130913510.20709@syhkavp.arg>

On Fri, Sep 13, 2013 at 8:27 PM, Nicolas Pitre <nico@fluxnic.net> wrote:

Show 12 quoted lines

> On Thu, 12 Sep 2013, Nguyễn Thái Ngọc Duy wrote:
>
>> The intention is to store flat v4 trees in delta base cache to avoid
>> repeatedly expanding copy sequences in v4 trees. When the user needs
>> to unpack a v4 tree and the tree is found in the cache, the tree will
>> be converted back to canonical format. Future tree_desc interface may
>> skip canonical format and read v4 trees directly.
>>
>> For that to work we need to keep track of v4 tree size after all copy
>> sequences are expanded, which is the purpose of this new field.
>
> Hmmm.... I think this is going in a wrong direction.

Good thing you caught me early. I was planning to implement a better version of this on the weekend. And you are not wrong about code maintainability, unpack_entry() so far looks very close to a real mess.

> Yet, pavkv4 tree walking shouldn't need a cache since there is nothing
> to expand in the end.  Entries should be advanced one by one as they are
> needed.  Granted when converting back to a canonical object we need all
> of them, but eventually this shouldn't be the main mode of operation.

There's another case where one of the base tree is not v4 (the packer is inefficient, like my index-pack --fix-thin). For trees with leading zeros in entry mode, we can just do a lossy conversion to v4, but I wonder if there is a case where we can't even convert to v4 and the v4 treewalk interface has to fall back to canonical format.. I guess that can't happen.

Show 10 quoted lines

> However I can see that, as you say, the same base object is repeatedly
> referenced.  This means that we need to parse it over and over again
> just to find the right offset where the needed entries start.  We
> probably end up skipping over the first entries in a tree object
> multiple times.  And that would be true even when the core code learns
> to walk pv4 trees directly.
>
> So here's the beginning of a tree offset cache to mitigate this problem.
> It is incomplete as the cache function is not implemented, etc.  But
> that should give you the idea.

Thanks. I'll have a closer look and maybe complete your patch.

-- 
Duy