Re: [PATCH 00/12] Die commit->util, die!
On Sat, May 12, 2018 at 1:00 AM, Nguyễn Thái Ngọc Duy wrote: > There's not much to write here. It's basically a copy from 12/12: > > This 'util' pointer can be used for many different purposes, > controlled in different ways. Some are not even contained in a command > code, but buried deep in common code with no clue who will use it and > how. For example, if revs.show_source is set, then it's used for > storing path name, but if you happen to call get_merge_parent() then > some 'util' may end up storing another thing. This is cool! It makes life easy when it comes to clearing up commits in the object store (as it is unclear what the util pointer is used for we leaked that memory so far in the object store conversion series). > The move to using commit-slab gives us a much better picture of how > some piece of data is associated with a commit and what for. Since > nobody uses 'util' pointer anymore, we can retire it so that nobody will > abuse it again. commit-slab will be the way forward for associating > data to a commit. > > As a side benefit, this shrinks struct commit by 8 bytes (on 64-bit > architecture) which should help reduce memory usage for reachability > test a bit. This is also what commit-slab is invented for [1]. The object store series (called -lookup-final) is at a point where I have to move one of the slabs (buffer_slab in commit.c) into the repository or rather into the parsed_object_pool in object.h. So I think I'll build a series on top of this one first, which allows for not just defining the slabs all over the place, but actually allows them to be embedded into other structs. Then I merge that series with origin/sb/object-store-graft and build the -final series on top of that. Stefan
Re: [PATCH 00/12] Die commit->util, die!
On Sat, May 12, 2018 at 8:50 PM, Jakub Narebski wrote: > I just wonder if most of those transformation could not be done with > Coccinelle, instead of doing it by hand. I doubt coccinelle is smart enough to figure out the convoluted use of 'util' pointer (but then I'm not a heavy coccinelle user). I actually got it wrong a couple times and had to rely on the test suite to guide me. -- Duy
Re: [PATCH 00/12] Die commit->util, die!
Nguyễn Thái Ngọc Duy writes: > There's not much to write here. It's basically a copy from 12/12: > > This 'util' pointer can be used for many different purposes, > controlled in different ways. Some are not even contained in a command > code, but buried deep in common code with no clue who will use it and > how. For example, if revs.show_source is set, then it's used for > storing path name, but if you happen to call get_merge_parent() then > some 'util' may end up storing another thing. > > The move to using commit-slab gives us a much better picture of how > some piece of data is associated with a commit and what for. Since > nobody uses 'util' pointer anymore, we can retire it so that nobody will > abuse it again. commit-slab will be the way forward for associating > data to a commit. > > As a side benefit, this shrinks struct commit by 8 bytes (on 64-bit > architecture) which should help reduce memory usage for reachability > test a bit. This is also what commit-slab is invented for [1]. > > [1] 96c4f4a370 (commit: allow associating auxiliary info on-demand - > 2013-04-09) Good work. This would reduce the amount of technical debt in Git. I just wonder if most of those transformation could not be done with Coccinelle, instead of doing it by hand. > Nguyễn Thái Ngọc Duy (12): > blame: use commit-slab for blame suspects instead of commit->util > describe: use commit-slab for commit names instead of commit->util > shallow.c: use commit-slab for commit depth instead of commit->util > sequencer.c: use commit-slab to mark seen commits > sequencer.c: use commit-slab to associate todo items to commits > revision.c: use commit-slab for show_source > bisect.c: use commit-slab for commit weight instead of commit->util > name-rev: use commit-slab for rev-name instead of commit->util > show-branch: use commit-slab for commit-name instead of commit->util > log: use commit-slab in prepare_bases() instead of commit->util > merge: use commit-slab in merge remote desc instead of commit->util > commit.h: delete 'util' field in struct commit > > bisect.c | 12 +--- > blame.c | 42 +++--- > blame.h | 2 ++ > builtin/blame.c | 2 +- > builtin/describe.c| 16 +--- > builtin/fast-export.c | 14 +- > builtin/log.c | 17 + > builtin/merge.c | 25 + > builtin/name-rev.c| 23 --- > builtin/show-branch.c | 39 +++ > commit.c | 12 ++-- > commit.h | 8 ++-- > log-tree.c| 8 ++-- > merge-recursive.c | 8 +--- > revision.c| 17 + > revision.h| 5 - > sequencer.c | 24 ++-- > shallow.c | 37 + > 18 files changed, 225 insertions(+), 86 deletions(-)
Re: [PATCH 00/12] Die commit->util, die!
On Sat, May 12, 2018 at 10:00:16AM +0200, Nguyễn Thái Ngọc Duy wrote: > There's not much to write here. It's basically a copy from 12/12: > > This 'util' pointer can be used for many different purposes, > controlled in different ways. Some are not even contained in a command > code, but buried deep in common code with no clue who will use it and > how. For example, if revs.show_source is set, then it's used for > storing path name, but if you happen to call get_merge_parent() then > some 'util' may end up storing another thing. > > The move to using commit-slab gives us a much better picture of how > some piece of data is associated with a commit and what for. Since > nobody uses 'util' pointer anymore, we can retire it so that nobody will > abuse it again. commit-slab will be the way forward for associating > data to a commit. > > As a side benefit, this shrinks struct commit by 8 bytes (on 64-bit > architecture) which should help reduce memory usage for reachability > test a bit. This is also what commit-slab is invented for [1]. I left a few comments, but overall this looks pretty good. A few of the conversions get tricky with the number of pointer dereferences, but most of those were pretty tricky to begin with (that weight stuff in bisect.c...yikes!). I love the result. More maintainable code, less possibility of conflicts in the util field, and a memory savings to boot. -Peff
[PATCH 00/12] Die commit->util, die!
There's not much to write here. It's basically a copy from 12/12: This 'util' pointer can be used for many different purposes, controlled in different ways. Some are not even contained in a command code, but buried deep in common code with no clue who will use it and how. For example, if revs.show_source is set, then it's used for storing path name, but if you happen to call get_merge_parent() then some 'util' may end up storing another thing. The move to using commit-slab gives us a much better picture of how some piece of data is associated with a commit and what for. Since nobody uses 'util' pointer anymore, we can retire it so that nobody will abuse it again. commit-slab will be the way forward for associating data to a commit. As a side benefit, this shrinks struct commit by 8 bytes (on 64-bit architecture) which should help reduce memory usage for reachability test a bit. This is also what commit-slab is invented for [1]. [1] 96c4f4a370 (commit: allow associating auxiliary info on-demand - 2013-04-09) Nguyễn Thái Ngọc Duy (12): blame: use commit-slab for blame suspects instead of commit->util describe: use commit-slab for commit names instead of commit->util shallow.c: use commit-slab for commit depth instead of commit->util sequencer.c: use commit-slab to mark seen commits sequencer.c: use commit-slab to associate todo items to commits revision.c: use commit-slab for show_source bisect.c: use commit-slab for commit weight instead of commit->util name-rev: use commit-slab for rev-name instead of commit->util show-branch: use commit-slab for commit-name instead of commit->util log: use commit-slab in prepare_bases() instead of commit->util merge: use commit-slab in merge remote desc instead of commit->util commit.h: delete 'util' field in struct commit bisect.c | 12 +--- blame.c | 42 +++--- blame.h | 2 ++ builtin/blame.c | 2 +- builtin/describe.c| 16 +--- builtin/fast-export.c | 14 +- builtin/log.c | 17 + builtin/merge.c | 25 + builtin/name-rev.c| 23 --- builtin/show-branch.c | 39 +++ commit.c | 12 ++-- commit.h | 8 ++-- log-tree.c| 8 ++-- merge-recursive.c | 8 +--- revision.c| 17 + revision.h| 5 - sequencer.c | 24 ++-- shallow.c | 37 + 18 files changed, 225 insertions(+), 86 deletions(-) -- 2.17.0.705.g3525833791
