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

How to substructure rewrites?

From
David Kastrup <dak@gnu.org>
Date
Jan 25, 2014, 12:44 UTC
Message-ID
<877g9ocjsk.fsf@fencepost.gnu.org>
Hi,

I'm currently rewriting the internals of git-blame. For the somewhat academic example

#/bin/sh
cd /tmp
rm -rf testit
mkdir testit
cd testit
git init
for inc in 8 4 3 7 2 9 5 6 1
do
    seq 252000 -$inc 0 > testfile
    git add testfile
    git commit -m "Run with increment $inc"
done
time git blame testfile >/dev/null

the performance of git-blame increases by 4000%. It turns out that in "real-world" bad cases, particularly after aggressively repacking repositories, the gains are quite more modest. I've seen a factor of 2 with source code and a factor of 3 with a large word list. The bulk of the remaining time is spent in system time (which has basically not changed by my work), so it is likely that the repository gets read over and over, calling for some better caching strategies.

So the question is how to restructure the work. I want to put out a "teaser" soonish but I think that there is little point in substructuring the commits I have so far. They start with a few janitorial patches already in pu, but after that there is

commit 7c610731e9452f3e8c175ab7e7542eafed1c5b93
Author: David Kastrup <dak@gnu.org>
Date:   Sat Jan 25 13:19:17 2014 +0100
    wip
commit dca4c8c447b94e1a3f8f45874d5415665d503189
Author: David Kastrup <dak@gnu.org>
Date:   Wed Jan 22 01:16:06 2014 +0100
    wip
commit f64b41c472442ae9971321fe8f62c3885ba4d8b7
Author: David Kastrup <dak@gnu.org>
Date:   Sun Jan 19 02:16:21 2014 +0100
    blame.c: Let output determine MORE_THAN_ONE_PATH more efficiently
commit 607b094537d6fe4a75cf9a297470966bda934c73
Author: David Kastrup <dak@gnu.org>
Date:   Mon Jan 13 18:06:06 2014 +0100
    wip
commit 1cd4e172533fb55793d02147c9596c59f7b05343
Author: David Kastrup <dak@gnu.org>
Date:   Sun Jan 19 01:30:02 2014 +0100
    Reorganize blame data structures
commit 5ab2add67d5fcf3837623a7bc028bcaec1006cc4
Author: David Kastrup <dak@gnu.org>
Date:   Sun Jan 19 01:29:28 2014 +0100
    Add blame_sort function for sorting a blame list
commit d3745752968617979878962ead957a6b5f829f0b
Author: David Kastrup <dak@gnu.org>
Date:   Sun Jan 19 01:28:22 2014 +0100
    Add merge_blame function for merging two sorted blame lists.

As it can easily be guessed, the "add xxx function" commits are basically adding not-yet-used code (and so will not disrupt compilation), but everything starting with "Reorganize blame data structures" up until the final commit will not work or compile since the code does not match the data structures.

So there is little point in substructing all that, right? Even something seemingly isolated like

commit f64b41c472442ae9971321fe8f62c3885ba4d8b7
Author: David Kastrup <dak@gnu.org>
Date:   Sun Jan 19 02:16:21 2014 +0100
    blame.c: Let output determine MORE_THAN_ONE_PATH more efficiently

is not really useful as a separate commit since while it does implement a particular task, this is done starting with non-working code relying on no-longer existent data structures.

I'm seriously considering squashing all that in a single commit. It seems like it would be utterly pointless to introduce artificially working in-between states that simulate a partial "migration" to new data structures that never happened. Where the algorithmic setup happened, I rewrite from scratch, and it's pretty absurd to match and marry efficient and inefficient half-algorithms.

Which brings us to point two: the "teaser" part. The current stuff has #if 0/#endif around the old implementation of the expensive -M and -C options. They are currently not operative due to the mismatch to data structures. Obviously, that makes the change unsuitable for putting into master or anywhere close.

I'd still want to solicit feedback: when not using those options (which will likely cause me another week of headaches), the code is working well and delivering identical information. So should I just squash the stuff and leave just the commits that made it into pu separate (so that people may git-am whether or not they already have those commits)?

In general, the rule is likely "any commit should not create a non-working state" right?

-- 
David Kastrup
Next: David Kastrup
Message 1 of 11 in “How to substructure rewrites?”
  1. David KastrupJan 25, 2014
  2. 0/3 "Teaser" patch for rewriting blame for efficiencyDavid Kastrup, Jan 25, 2014
  3. 1/3 builtin/blame.c: struct blame_entry does not need a prev linkDavid Kastrup, Jan 25, 2014
  4. 2/3 Eliminate same_suspect function in builtin/blame.cDavid Kastrup, Jan 25, 2014
  5. 3/3 builtin/blame.c: large-scale rewriteDavid Kastrup, Jan 25, 2014
  6. Junio C HamanoJan 27, 2014
  7. David KastrupJan 27, 2014
  8. Junio C HamanoJan 27, 2014
  9. David KastrupJan 27, 2014
  10. Junio C HamanoJan 27, 2014
  11. David KastrupJan 27, 2014

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.