git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 0/7] Better threaded delta resolution in index-pack

From
Josh Steadmon <steadmon@google.com>
Date
Feb 28, 2020, 00:03 UTC
Message-ID
<20200228000350.GB12115@google.com>
In-Reply-To
<cover.1571343096.git.jonathantanmy@google.com>
On 2019.10.17 13:17, Jonathan Tan wrote:
Show 44 quoted lines
> Thanks, Stolee and Peff, for taking a look at it. Here is a v2. It is
> mostly unchanged, except for expanded commit messages and code comments.
> 
> I've also added a documentation clarification that
> core.deltaBaseCacheLimit is per-thread, appearing as the first patch in
> this patch series.
> 
> From patch 3 (now patch 4):
> 
> > > +	int i;
> > 
> > Technically this probably ought to be a size_t as well, but I'm much
> > more concerned about the allocation ones, where we might accidentally
> > overflow and underallocate a buffer. Overflowing "i" would probably just
> > lead to an error or bad result.
> 
> I believe this needs to be signed, since we're iterating in reverse
> order, so I made it a ssize_t instead (note the extra "s" in front).
> 
> From patch 4 (now patch 5):
> 
> > > Whenever we make a struct base_data, immediately calculate its delta
> > > children. This eliminates confusion as to when the
> > > {ref,ofs}_{first,last} fields are initialized.
> > 
> > That _seems_ like a good idea, but I'm a little worried just because I
> > don't entirely understand why it was being done lazily before. If you've
> > puzzled all that out, it would be nice to make the argument in the
> > commit message.
> 
> I've added an explanation in the commit message.
> 
> Jonathan Tan (7):
>   Documentation: deltaBaseCacheLimit is per-thread
>   index-pack: unify threaded and unthreaded code
>   index-pack: remove redundant parameter
>   index-pack: remove redundant child field
>   index-pack: calculate {ref,ofs}_{first,last} early
>   index-pack: make resolve_delta() assume base data
>   index-pack: make quantum of work smaller
> 
>  Documentation/config/core.txt |   2 +-
>  builtin/index-pack.c          | 446 ++++++++++++++++++----------------
>  2 files changed, 242 insertions(+), 206 deletions(-)

This series mostly looks good to me. There were a few parts I had trouble following or convincing myself were safe, so there could be some improvements with comments or more explicit commit messages, but no problems apart from that.

Previous: Jonathan TanNext: Jonathan Tan
Message 30 of 31 in “Better threaded delta resolution in index-pack”
  1. 0/6 Better threaded delta resolution in index-packJonathan Tan, Oct 9, 2019
  2. 1/6 index-pack: unify threaded and unthreaded codeJonathan Tan, Oct 9, 2019
  3. Jeff KingOct 17, 2019
  4. 2/6 index-pack: remove redundant parameterJonathan Tan, Oct 9, 2019
  5. Jeff KingOct 17, 2019
  6. 3/6 index-pack: remove redundant child fieldJonathan Tan, Oct 9, 2019
  7. Derrick StoleeOct 10, 2019
  8. Jonathan TanOct 10, 2019
  9. Jeff KingOct 17, 2019
  10. Jeff KingOct 17, 2019
  11. 4/6 index-pack: calculate {ref,ofs}_{first,last} earlyJonathan Tan, Oct 9, 2019
  12. Jeff KingOct 17, 2019
  13. 5/6 index-pack: make resolve_delta() assume base dataJonathan Tan, Oct 9, 2019
  14. Jeff KingOct 17, 2019
  15. 6/6 index-pack: make quantum of work smallerJonathan Tan, Oct 9, 2019
  16. Jeff KingOct 17, 2019
  17. 0/7 Better threaded delta resolution in index-packJonathan Tan, Oct 17, 2019
  18. 1/7 Documentation: deltaBaseCacheLimit is per-threadJonathan Tan, Oct 17, 2019
  19. 3/7 index-pack: remove redundant parameterJonathan Tan, Oct 17, 2019
  20. Josh SteadmonFeb 28, 2020
  21. Jonathan TanMar 10, 2020
  22. 2/7 index-pack: unify threaded and unthreaded codeJonathan Tan, Oct 17, 2019
  23. 4/7 index-pack: remove redundant child fieldJonathan Tan, Oct 17, 2019
  24. Josh SteadmonFeb 28, 2020
  25. 5/7 index-pack: calculate {ref,ofs}_{first,last} earlyJonathan Tan, Oct 17, 2019
  26. 6/7 index-pack: make resolve_delta() assume base dataJonathan Tan, Oct 17, 2019
  27. 7/7 index-pack: make quantum of work smallerJonathan Tan, Oct 17, 2019
  28. Josh SteadmonFeb 28, 2020
  29. Jonathan TanMar 10, 2020
  30. Josh SteadmonFeb 28, 2020
  31. Jonathan TanMar 10, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.