Re: [PATCH v3 11/14] commit: integrate commit graph with commit parsing
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Feb 14, 2018, 18:08 UTC
- Message-ID
- <5e53ef79-e196-e524-194c-741851db0755@gmail.com>
- In-Reply-To
- <20180213161225.6e21291d25015be632b03fee@google.com>
On 2/13/2018 7:12 PM, Jonathan Tan wrote:
Show 10 quoted lines
> On Thu, 8 Feb 2018 15:37:35 -0500 > Derrick Stolee <stolee@gmail.com> wrote: > >> | Command | Before | After | Rel % | >> |----------------------------------|--------|--------|-------| >> | log --oneline --topo-order -1000 | 5.9s | 0.7s | -88% | >> | branch -vv | 0.42s | 0.27s | -35% | >> | rev-list --all | 6.4s | 1.0s | -84% | >> | rev-list --all --objects | 32.6s | 27.6s | -15% | > Could we have a performance test (in t/perf) demonstrating this?
The rev-list perf tests are found in t/perf/p0001-rev-list.sh
The "log --oneline --topo-order -1000" test would be good to add to t/perf/p4211-line-log.sh
The "branch -vv" test is pretty uninteresting unless you set up your repo to have local branches significantly behind the remote branches. It depends a lot more on the data shape than the others which only need a large number of reachable objects.
One reason I did not use the builtin perf test scripts is that they seem to ignore all local config options, and hence do not inherit the core.commitGraph=true setting from the repos pointed at by GIT_PERF_REPO.
Show 26 quoted lines
>
>> +static int check_commit_parents(struct commit *item, struct commit_graph *g,
>> + uint32_t pos, const unsigned char *commit_data)
> Document what this function does? Also, this function probably needs a
> better name.
>
>> +/*
>> + * Given a commit struct, try to fill the commit struct info, including:
>> + * 1. tree object
>> + * 2. date
>> + * 3. parents.
>> + *
>> + * Returns 1 if and only if the commit was found in the commit graph.
>> + *
>> + * See parse_commit_buffer() for the fallback after this call.
>> + */
>> +int parse_commit_in_graph(struct commit *item)
>> +{
> The documentation above duplicates what's in the header file, so we can
> probably omit it.
>
>> +extern struct object_id *get_nth_commit_oid(struct commit_graph *g,
>> + uint32_t n,
>> + struct object_id *oid);
> This doesn't seem to be used elsewhere - do you plan for a future patch
> to use it?