patchblame: remove unnecessary use of get_commit_info()

This patch is a code cleanup and consequently a micro optimization in determine_heat_line().

When `blame`ing with --color-by-age, the determine_line_heat() calls the get_commit_info() to parse the commit's information. However, it turns out this is actually unnecessary because its caller also does the same. We can instead, pass the already parsed `commit_info` and remove the internal call to get_commit_info().

The performance optimization made by this patch is relatively small as get_commit_info() doesn't perform any heavy-load operations. So, the performance improvements is not that exciting, not at least until you enter the ms/μs realm. Nevertheless I thought the code cleanup is still valid and the optimization is a bonus.

  ... digging into the the performance improvements, for those 
      who are curious ...

Running Git PERF suite in linux.git, I've got a subtle performance improvement for some runs:

	# git.328c109303 - compiled git from commit 328c109303
	# git.blame-patched - compiled git from commit 328c109303 + this patch
        Test                                          git.328c109303    git.blame-patched
        -------------------------------------------------------------------------------------
        blame --color-by-age kernel/fork.c            1.96(1.81+0.15)   1.95(1.80+0.14) -0.5%
        blame --color-by-age kernel/sys.c             1.67(1.53+0.13)   1.66(1.52+0.14) -0.6%
        blame --color-by-age mm/slab.c                2.16(2.00+0.16)   2.15(1.99+0.15) -0.5%
        blame --color-by-age lib/packing.c            0.20(0.14+0.05)   0.20(0.14+0.05) +0.0%
        blame --color-by-age drivers/cdrom/cdrom.c    1.62(1.46+0.15)   1.62(1.46+0.15) +0.0%
        blame --color-by-age crypto/crypto_engine.c   0.37(0.29+0.06)   0.36(0.28+0.06) -2.7%
        blame --color-by-age net/ipv4/ip_forward.c    1.49(1.35+0.13)   1.48(1.34+0.13) -0.7%

To dig a little deeper, I enabled the Git's trace2 API to record every call to the determine_line_heat() function:

        ...
+       trace2_region_enter("blame", "determine_line_heat", the_repository);
        determine_line_heat(ent, &default_color);
+       trace2_region_enter("blame", "determine_line_heat", the_repository);
        ...

Then, running `blame` for "kernel/fork.c` and _summing_ all the execution time for every call (around 1.3k calls) resulted in 2.6x faster execution (best out 3):

	git built from 328c109303 (The eighth batch, 2021-02-12) = 42ms
	git built from 328c109303 + this patch                   = 16ms

Of course, this is on the milliseconds :).

Lastly, to get a better picture of the performance change, I computed all the determine_line_heat()'s execution time, for all 3 calls, into a distribution graph. The execution time is converted to microseconds for ease of understanding:

        # git built from 328c109303 (The eighth batch, 2021-02-12)
     	Time in μs     0.0 ~  9.0  |
     	Time in μs    10.0 ~ 19.0  | *
     	Time in μs    20.0 ~ 29.0  | *****************
    	Time in μs    30.0 ~ 39.0  | *****************
     	Time in μs    40.0 ~ 49.0  | **
     	Time in μs    50.0 ~ 59.0  | *
     	Time in μs    60.0 ~ 69.0  | *
     	Time in μs    70.0 ~ 79.0  | *
     	Time in μs    80.0 ~ 89.0  | *
     	Time in μs    90.0 ~ 99.0  | *
     	Time in μs   100.0 ~ 109.0 | *
     	Time in μs   110.0 ~ 119.0 | *
     	Time in μs   120.0 ~ 129.0 |
     	Time in μs   130.0 ~ 139.0 | *
     	[N: 3915 | Median: 30.0 | Min: 19.0 | Max: 137.0]

     	# git built from 328c109303 + this patch
     	Time in μs     0.0 ~  9.0  |
     	Time in μs    10.0 ~ 19.0  | *************************************
     	Time in μs    20.0 ~ 29.0  | *
     	Time in μs    30.0 ~ 39.0  | *
     	Time in μs    40.0 ~ 49.0  | *
     	Time in μs    50.0 ~ 59.0  | *
     	Time in μs    60.0 ~ 69.0  | *
     	Time in μs    70.0 ~ 79.0  | *
     	Time in μs    80.0 ~ 89.0  | *
     	Time in μs    90.0 ~ 99.0  |
	        ... [ zero's omitted ] ...
     	Time in μs   150.0 ~ 159.0 |
     	Time in μs   160.0 ~ 169.0 | *
     	[N: 3915 | Median: 12.0 | Min: 10.0 | Max: 167.0]

	Note: A "*" represent hundreds counter. One "*" is used
	for any count between 1 and 99. (i.e: 23 = "*", 99 = "*",
	100 = "**", 199 = "**", 200 = "***", etc).

With the changes introduced by this patch, the determine_line_heat() mostly finishes its execution between 10μs and 19μs compared with the current implementation that mostly finished around 20μs and 39μs.

Rafael Silva (1):
  blame: remove unnecessary use of get_commit_info()

 builtin/blame.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

When `git blame --color-by-age`, the determine_line_heat() is called to select how to color the output based on the commit author's date. It uses the get_commit_info() to parse the information into a `commit_info` structure, however, this is actually unnecessary because the determine_line_heat() caller also does the same.

Instead, let's change determine_line_heat() to take a `commit_info` structure and remove the internal call to get_commit_info() thus cleaning up and micro-optimizing the code path.

Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
 builtin/blame.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

On Tue, Feb 16, 2021 at 05:31:51PM +0100, Rafael Silva wrote:

Interesting. It looks like this micro-optimization could have been safely performed as early as 25d5f52901 (builtin/blame: highlight recently changed lines, 2018-04-23), which is when this feature was originally introduced.

I looked at 25d5f52901 to see if there was any reason that we didn't at the time, but couldn't find anything.

So this looks correct to me. I'm a little disappointed that some of your more detailed performance numbers from the cover letter didn't make it into the patch description, but it may not be worth belaboring the point further.

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

Thanks for reviewing this patch.

I wasn't sure whether adding the performance number into the patch was a good idea or not. After reading your message and the response from Derrick and Junio, I'll definitely re-roll this patch adding those information.

On 2/16/2021 11:31 AM, Rafael Silva wrote:

Have you updated the commit-graph with changed-path Bloom filters in your copy of linux.git before running the perf tests? You might get smaller numbers overall (both sides) but make the difference for this patch be more pronounced:

	git commit-graph write --reachable --changed-paths

This is a good way to demonstrate the change. Definitely worthwhile for demonstrating the value of the patch. I'll second Taylor's request that this performance data goes in the commit message so we can see the details in the future.

Thanks, -Stolee

Derrick Stolee <stolee@gmail.com> writes:

Thanks, all. I do agree with you and Taylor that these numbers deserve to be in the log message, not cover letter.

Derrick Stolee <stolee@gmail.com> writes:

Thanks for pointing this out. I didn't updated the commit-graph in my copy of linux.git when I ran the perf tests and indeed with the commit-graph updated, the execution time is faster now.

Here's the results (best out of 3):

        # git.328c109303 - compiled git from commit 328c109303
        # git.blame-patched - compiled git from commit 328c109303 + this patch
        Test                                          git.328c109303   git.blame-patched
        ------------------------------------------------------------------------------------
        blame --color-by-age kernel/fork.c            1.13(0.96+0.17)  1.12(0.96+0.15) -0.9%
        blame --color-by-age kernel/sys.c             1.00(0.81+0.18)  0.98(0.81+0.15) -2.0%
        blame --color-by-age mm/slab.c                1.51(1.33+0.18)  1.49(1.31+0.17) -1.3%
        blame --color-by-age lib/packing.c            0.13(0.05+0.07)  0.12(0.04+0.07) -7.7%
        blame --color-by-age drivers/cdrom/cdrom.c    0.62(0.48+0.12)  0.61(0.48+0.12) -1.6%
        blame --color-by-age crypto/crypto_engine.c   0.16(0.08+0.08)  0.16(0.07+0.08) +0.0%
        blame --color-by-age net/ipv4/ip_forward.c    0.43(0.30+0.13)  0.42(0.29+0.11) -2.3%

For the "kernel/fork.c" example, there is around 1.7x improvement. The percentage comparison is also bigger than it was before the commit-graph file was updated with the changed paths Bloom Filter as you expected.

For the "net/ipv4/ip_forward.c" there a 3.4x improvements which I believe is the biggest one in my tests.

Thanks Taylor, Derrick and Junio for reviewing v1. I've updated the patch's message to include one of the performance reports that was previously only on the cover-letter as suggested.

Rafael Silva (1):
  blame: remove unnecessary use of get_commit_info()

 builtin/blame.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Range-diff against v1:
1:  e0c698370e ! 1:  4d70ce9545 blame: remove unnecessary use of get_commit_info()
    @@ Commit message
         structure and remove the internal call to get_commit_info() thus
         cleaning up and optimizing the code path.
     
    +    Enabling Git's trace2 API in order to record the execution time for
    +    every call to determine_line_heat() function:
    +
    +       + trace2_region_enter("blame", "determine_line_heat", the_repository);
    +         determine_line_heat(ent, &default_color);
    +       + trace2_region_enter("blame", "determine_line_heat", the_repository);
    +
    +    Then, running `git blame` for "kernel/fork.c" in linux.git and summing
    +    all the execution time for every call (around 1.3k calls) resulted in
    +    2.6x faster execution (best out 3):
    +
    +       git built from 328c109303 (The eighth batch, 2021-02-12) = 42ms
    +       git built from 328c109303 + this change                  = 16ms
    +
         Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
     
      ## builtin/blame.c ##

When `git blame --color-by-age`, the determine_line_heat() is called to select how to color the output based on the commit's author date. It uses the get_commit_info() to parse the information into a `commit_info` structure, however, this is actually unnecessary because the determine_line_heat() caller also does the same.

Instead, let's change the determine_line_heat() to take a `commit_info` structure and remove the internal call to get_commit_info() thus cleaning up and optimizing the code path.

Enabling Git's trace2 API in order to record the execution time for every call to determine_line_heat() function:

   + trace2_region_enter("blame", "determine_line_heat", the_repository);
     determine_line_heat(ent, &default_color);
   + trace2_region_enter("blame", "determine_line_heat", the_repository);

Then, running `git blame` for "kernel/fork.c" in linux.git and summing all the execution time for every call (around 1.3k calls) resulted in 2.6x faster execution (best out 3):

   git built from 328c109303 (The eighth batch, 2021-02-12) = 42ms
   git built from 328c109303 + this change                  = 16ms

Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
 builtin/blame.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)