Re: [RFC PATCH] fetch-pack: space out sent "haves" in negotiation

From: Stefan Beller <sbeller@google.com>
Date: May 22, 2018, 19:01 UTC
Message-ID: <CAGZ79kbe0mAQW-2iGX2Q+WEWJ-MEoBcXtyBOb8a-7iWApp7pqw@mail.gmail.com>
In-Reply-To: <20180522114440.0b1aa23a780efe6a68770d9b@google.com>

Hi Jonathan,

> I wouldn't characterize the errors as "off by one errors".

Yes, I put it in quotes but realized that would not convey it very well.

Show 14 quoted lines

>  They are
> more like...let me use a diagram:
>
> A
> |\
> B D
> | |
> C E
>
> Suppose we know that the server does not have A, has C, and may or may
> not have E (we sent "have E" but didn't get a response yet). My method
> restarts the walk at all the parents of A (that is, B and D), but D is
> irrelevant to the situation (and should not be walked over - this is the
> error).

D is irrelevant to the A, C situation, but it is still a useful probing point? So I would not call it an error but an inefficiency?

> In this patch, I wrote the new algorithm and deleted the old one.

...

Show 5 quoted lines

> You're proposing that if I proceed with this, I split the patch into 2 -
> one to move the negotiation algorithm, and one to update it? If yes,
> normally I would agree, but the current negotiation algorithm is not
> very sophisticated (and does not take up much code), so I think it's not
> worth it.

ok, in that case I'll just dive into the code.

Show 13 quoted lines

>
>> > +struct fetch_negotiator {
>> > +       struct sent_commit **sent_commits;
>> > +       size_t sent_commit_nr, sent_commit_alloc;
>> > +       struct prio_queue candidates;
>> > +};
>>
>> Maybe we can just declare the struct fetch_negotiator here and not
>> define it, such that nobody outside the actual implementation tries
>> to access its internals?
>
> That's possible - I wanted to allow allocation of this on the stack (to
> save a malloc), but perhaps that's over-optimization.

Ah good call. Please keep it that way then.

Show 5 quoted lines

>> So even if we do not use the skip commit logic, this would be a benefit for any
>> http(-v0) and v2 users of the protocol?
>
> It would conserve bandwidth, yes, but storing all the commits sent with
> additional metadata for each would require more memory.

I did not see the memory requirements here as a problem until now. Are you saying this memory might be too much to keep around?

Stefan