From: Junio C Hamano <hidden> Date: 2016-08-09 16:52:26
Kirill Smelkov [off-list ref] writes:
Would you please explain why we should not use touch if we do not care
about timestamps? Simply style?
To help readers.
"touch A" forcess the readers wonder "does the timestamp of A
matter, and if so in what way?" and "does any later test care what
is _in_ A, and if so in what way?" Both of them is wasting their
time when there is no reason why "touch" should have been used.
@@ -8,16 +8,15 @@ objpath () {}# show objects present in pack ($1 should be associated *.idx)-packobjects(){-gitshow-index<$1|cut-d' '-f2+pack_list_objects(){+gitshow-index<"$1"|cut-d' '-f2}
pack-list-objects still sounds as if you are packing "list objects",
though. If you are listing packed objects (or objects in a pack),
list-packed-objects (or list-objects-in-pack) reads clearer and more
to the point, at least to me.
-# hasany pattern-file content-file
+# has_any pattern-file content-file
# tests whether content-file has any entry from pattern-file with entries being
# whole lines.
-hasany () {
- # NOTE `grep -f` is not portable
- git grep --no-index -qFf $1 $2
+has_any () {
+ grep -qFf "$1" "$2"
Omitting "-q" would help those who have to debug breakage in this
test or the code that this test checks. What test_expect_success
outputs is not shown by default, and running the test script with
"-v" would show them as a debugging aid.
Thanks.
On Tue, Aug 09, 2016 at 09:52:18AM -0700, Junio C Hamano wrote:
Kirill Smelkov [off-list ref] writes:
quoted
Would you please explain why we should not use touch if we do not care
about timestamps? Simply style?
To help readers.
"touch A" forcess the readers wonder "does the timestamp of A
matter, and if so in what way?" and "does any later test care what
is _in_ A, and if so in what way?" Both of them is wasting their
time when there is no reason why "touch" should have been used.
I see, thanks for explaining. I used to read it a bit the other way;
maybe it is just an environment difference.
@@ -8,16 +8,15 @@ objpath () {}# show objects present in pack ($1 should be associated *.idx)-packobjects(){-gitshow-index<$1|cut-d' '-f2+pack_list_objects(){+gitshow-index<"$1"|cut-d' '-f2}
pack-list-objects still sounds as if you are packing "list objects",
though. If you are listing packed objects (or objects in a pack),
list-packed-objects (or list-objects-in-pack) reads clearer and more
to the point, at least to me.
Ok, let it be list_packed_objects().
quoted
-# hasany pattern-file content-file
+# has_any pattern-file content-file
# tests whether content-file has any entry from pattern-file with entries being
# whole lines.
-hasany () {
- # NOTE `grep -f` is not portable
- git grep --no-index -qFf $1 $2
+has_any () {
+ grep -qFf "$1" "$2"
Omitting "-q" would help those who have to debug breakage in this
test or the code that this test checks. What test_expect_success
outputs is not shown by default, and running the test script with
"-v" would show them as a debugging aid.
Ok, makes sense. Both patches adjusted and will be reposted.
Thanks,
Kirill
Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there
are two codepaths in pack-objects: with & without using bitmap
reachability index.
However add_object_entry_from_bitmap(), despite its non-bitmapped
counterpart add_object_entry(), in no way does check for whether --local
or --honor-pack-keep or --incremental should be respected. In
non-bitmapped codepath this is handled in want_object_in_pack(), but
bitmapped codepath has simply no such checking at all.
The bitmapped codepath however was allowing to pass in all those options
and with bitmap indices still being used under such conditions -
potentially giving wrong output (e.g. including objects from non-local or
.keep'ed pack).
We can easily fix this by noting the following: when an object comes to
add_object_entry_from_bitmap() it can come for two reasons:
1. entries coming from main pack covered by bitmap index, and
2. object coming from, possibly alternate, loose or other packs.
"2" can be already handled by want_object_in_pack() and to cover
"1" we can teach want_object_in_pack() to expect that *found_pack can be
non-NULL, meaning calling client already found object's pack entry.
In want_object_in_pack() we care to start the checks from already found
pack, if we have one, this way determining the answer right away
in case neither --local nor --honour-pack-keep are active. In
particular, as p5310-pack-bitmaps.sh shows, we do not do harm to
served-with-bitmap clones performance-wise:
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.63(8.67+0.33) 9.47(8.55+0.28) -1.7%
5310.3: simulated clone 2.07(2.17+0.12) 2.03(2.14+0.12) -1.9%
5310.4: simulated fetch 0.78(1.03+0.02) 0.76(1.00+0.03) -2.6%
5310.6: partial bitmap 1.97(2.43+0.15) 1.92(2.36+0.14) -2.5%
with all differences strangely showing we are a bit faster now, but
probably all being within noise.
And in the general case we care not to have duplicate
find_pack_entry_one(*found_pack) calls. Worst what can happen is we can
call want_found_object(*found_pack) -- newly introduced helper for
checking whether we want object -- twice, but since want_found_object()
is very lightweight it does not make any difference.
I appreciate help and discussing this change with Junio C Hamano and
Jeff King.
Signed-off-by: Kirill Smelkov <redacted>
---
builtin/pack-objects.c | 93 +++++++++++++++++++++++++++++++------------------
t/t5310-pack-bitmaps.sh | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 152 insertions(+), 33 deletions(-)
@@ -944,13 +944,44 @@ static int have_duplicate_entry(const unsigned char *sha1,return1;}+staticintwant_found_object(intexclude,structpacked_git*p)+{+if(exclude)+return1;+if(incremental)+return0;++/*+*Whenaskedtodo--local(donotincludeanobjectthatappearsina+*packweborrowfromelsewhere)or--honor-pack-keep(donotinclude+*anobjectthatappearsinapackmarkedwith.keep),findingapack+*thatmatchesthecriteriaissufficientforustodecidetoomitit.+*However,evenifthispackdoesnotsatisfythecriteria,weneedto+*makesurenocopyofthisobjectappearsin_any_packthatmakesus+*toomittheobject,soweneedtocheckallthepacks.Signalthatby+*returning-1tothecaller.+*/+if(!ignore_packed_keep&&+(!local||!have_non_local_packs))+return1;++if(local&&!p->pack_local)+return0;+if(ignore_packed_keep&&p->pack_local&&p->pack_keep)+return0;++/* we don't know yet; keep looking for more packs */+return-1;+}+/**Checkwhetherwewanttheobjectinthepack(e.g.,wedonotwant*objectsfoundinnon-localstoresifthe"--local"optionwasused).*-*Asasideeffectofthischeck,wewillfindthepackedversionofthis-*object,ifany.Wethereforepassoutthepackinformationtoavoidhaving-*tolookitupagainlater.+*Ifthecalleralreadyknowsanexistingpackitwantstotaketheobject+*from,thatispassedin*found_packand*found_offset;otherwisethis+*functionfindsifthereisanypackthathastheobjectandreturnsthepack+*anditsoffsetinthesevariables.*/staticintwant_object_in_pack(constunsignedchar*sha1,intexclude,
@@ -958,15 +989,30 @@ static int want_object_in_pack(const unsigned char *sha1,off_t*found_offset){structpacked_git*p;+intwant;if(!exclude&&local&&has_loose_object_nonlocal(sha1))return0;-*found_pack=NULL;-*found_offset=0;+/*+*Ifwealreadyknowthepackobjectlivesin,startchecksfromthat+*pack-intheusualcasewhenneither--localwasgivennor.keepfiles+*arepresentwewilldeterminetheanswerrightnow.+*/+if(*found_pack){+want=want_found_object(exclude,*found_pack);+if(want!=-1)+returnwant;+}for(p=packed_git;p;p=p->next){-off_toffset=find_pack_entry_one(sha1,p);+off_toffset;++if(p==*found_pack)+offset=*found_offset;+else+offset=find_pack_entry_one(sha1,p);+if(offset){if(!*found_pack){if(!is_pack_valid(p))
@@ -974,31 +1020,9 @@ static int want_object_in_pack(const unsigned char *sha1,*found_offset=offset;*found_pack=p;}-if(exclude)-return1;-if(incremental)-return0;--/*-*Whenaskedtodo--local(donotincludean-*objectthatappearsinapackweborrow-*fromelsewhere)or--honor-pack-keep(donot-*includeanobjectthatappearsinapackmarked-*with.keep),weneedtomakesurenocopyofthis-*objectcomefromin_any_packthatcausesusto-*omitit,andneedtocompletethisloop.When-*neitheroptionisineffect,weknowtheobject-*wejustfoundisgoingtobepacked,sobreak-*outofthelooptoreturn1now.-*/-if(!ignore_packed_keep&&-(!local||!have_non_local_packs))-break;--if(local&&!p->pack_local)-return0;-if(ignore_packed_keep&&p->pack_local&&p->pack_keep)-return0;+want=want_found_object(exclude,p);+if(want!=-1)+returnwant;}}
@@ -7,6 +7,18 @@ objpath () {echo".git/objects/$(echo"$1"|sed-e's|\(..\)|\1/|')"}+# show objects present in pack ($1 should be associated *.idx)+list_packed_objects(){+gitshow-index<"$1"|cut-d' '-f2+}++# has_any pattern-file content-file+# tests whether content-file has any entry from pattern-file with entries being+# whole lines.+has_any(){+grep-Ff"$1""$2"+}+ test_expect_success'setup repo with moderate-sized history''foriin$(test_seq110);dotest_commit$i
Starting from 6b8fda2d (pack-objects: use bitmaps when packing objects)
if a repository has bitmap index, pack-objects can nicely speedup
"Counting objects" graph traversal phase. That however was done only for
case when resultant pack is sent to stdout, not written into a file.
The reason here is for on-disk repack by default we want:
- to produce good pack (with bitmap index not-yet-packed objects are
emitted to pack in suboptimal order).
- to use more robust pack-generation codepath (avoiding possible
bugs in bitmap code and possible bitmap index corruption).
Jeff King further explains:
The reason for this split is that pack-objects tries to determine how
"careful" it should be based on whether we are packing to disk or to
stdout. Packing to disk implies "git repack", and that we will likely
delete the old packs after finishing. We want to be more careful (so
as not to carry forward a corruption, and to generate a more optimal
pack), and we presumably run less frequently and can afford extra CPU.
Whereas packing to stdout implies serving a remote via "git fetch" or
"git push". This happens more frequently (e.g., a server handling many
fetching clients), and we assume the receiving end takes more
responsibility for verifying the data.
But this isn't always the case. One might want to generate on-disk
packfiles for a specialized object transfer. Just using "--stdout" and
writing to a file is not optimal, as it will not generate the matching
pack index.
So it would be useful to have some way of overriding this heuristic:
to tell pack-objects that even though it should generate on-disk
files, it is still OK to use the reachability bitmaps to do the
traversal.
So we can teach pack-objects to use bitmap index for initial object
counting phase when generating resultant pack file too:
- if we care it is not activated under git-repack:
See above about repack robustness and not forward-carrying corruption.
- if we know bitmap index generation is not enabled for resultant pack:
Current code has singleton bitmap_git so cannot work simultaneously
with two bitmap indices.
We also want to avoid (at least with current implementation)
generating bitmaps off of bitmaps. The reason here is: when generating
a pack, not-yet-packed objects will be emitted into pack in
suboptimal order and added to tail of the bitmap as "extended entries".
When the resultant pack + some new objects in associated repository
are in turn used to generate another pack with bitmap, the situation
repeats: new objects are again not emitted optimally and just added to
bitmap tail - not in recency order.
So the pack badness can grow over time when at each step we have
bitmapped pack + some other objects. That's why we want to avoid
generating bitmaps off of bitmaps, not to let pack badness grow.
- if we keep pack reuse enabled still only for "send-to-stdout" case:
Because on pack reuse raw entries are directly written out to destination
pack by write_reused_pack() bypassing needed for pack index generation
bookkeeping done by regular codepath in write_one() and friends.
This way for pack-objects -> file we get nice speedup:
erp5.git[1] (~230MB) extracted from ~ 5GB lab.nexedi.com backup
repository managed by git-backup[2] via
time echo 0186ac99 | git pack-objects --revs erp5pack
before: 37.2s
after: 26.2s
And for `git repack -adb` packed git.git
time echo 5c589a73 | git pack-objects --revs gitpack
before: 7.1s
after: 3.6s
i.e. it can be 30% - 50% speedup for pack extraction.
git-backup extracts many packs on repositories restoration. That was my
initial motivation for the patch.
[1] https://lab.nexedi.com/nexedi/erp5
[2] https://lab.nexedi.com/kirr/git-backup
NOTE
Jeff also suggests that pack.useBitmaps was probably a mistake to
introduce originally. This way we are not adding another config point,
but instead just always default to-file pack-objects not to use bitmap
index: Tools which need to generate on-disk packs with using bitmap, can
pass --use-bitmap-index explicitly. And git-repack does never pass
--use-bitmap-index, so this way we can be sure regular on-disk repacking
remains robust.
NOTE2
`git pack-objects --stdout >file.pack` + `git index-pack file.pack` is much slower
than `git pack-objects file.pack`. Extracting erp5.git pack from
lab.nexedi.com backup repository:
$ time echo 0186ac99 | git pack-objects --stdout --revs >erp5pack-stdout.pack
real 0m22.309s
user 0m21.148s
sys 0m0.932s
$ time git index-pack erp5pack-stdout.pack
real 0m50.873s <-- more than 2 times slower than time to generate pack itself!
user 0m49.300s
sys 0m1.360s
So the time for
`pack-object --stdout >file.pack` + `index-pack file.pack` is 72s,
while
`pack-objects file.pack` which does both pack and index is 27s.
And even
`pack-objects --no-use-bitmap-index file.pack` is 37s.
Jeff explains:
The packfile does not carry the sha1 of the objects. A receiving
index-pack has to compute them itself, including inflating and applying
all of the deltas.
that's why for `git-backup restore` we want to teach `git pack-objects
file.pack` to use bitmaps instead of using `git pack-objects --stdout
@@ -2818,7 +2819,23 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)if(!rev_list_all||!rev_list_reflog||!rev_list_index)unpack_unreachable_expiration=0;-if(!use_internal_rev_list||!pack_to_stdout||is_repository_shallow())+/*+*"soft"reasonsnottousebitmaps-foron-diskrepackbydefaultwewant+*+*-toproducegoodpack(withbitmapindexnot-yet-packedobjectsare+*packedinsuboptimalorder).+*+*-tousemorerobustpack-generationcodepath(avoidingpossible+*bugsinbitmapcodeandpossiblebitmapindexcorruption).+*/+if(!pack_to_stdout)+use_bitmap_index_default=0;++if(use_bitmap_index<0)+use_bitmap_index=use_bitmap_index_default;++/* "hard" reasons not to use bitmaps; these just won't work at all */+if(!use_internal_rev_list||(!pack_to_stdout&&write_bitmap_index)||is_repository_shallow())use_bitmap_index=0;if(pack_to_stdout||!rev_list_all)
@@ -196,6 +196,18 @@ test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '!has_anypackbitmap.objects3b.objects'+test_expect_success'pack-objects to file can use bitmap''+# make sure we still have 1 bitmap index from previous tests+ls.git/objects/pack/|grepbitmap>output&&+test_line_count=1output&&+# verify equivalent packs are generated with/without using bitmap index+packasha1=$(gitpack-objects--no-use-bitmap-index--allpacka</dev/null)&&+packbsha1=$(gitpack-objects--use-bitmap-index--allpackb</dev/null)&&+list_packed_objects<packa-$packasha1.idx>packa.objects&&+list_packed_objects<packb-$packbsha1.idx>packb.objects&&+test_cmppacka.objectspackb.objects+'+ test_expect_success'full repack, reusing previous bitmaps''gitrepack-ad&&ls.git/objects/pack/|grepbitmap>output&&
From: Jeff King <hidden> Date: 2016-08-19 01:12:56
On Tue, Aug 09, 2016 at 10:31:43PM +0300, Kirill Smelkov wrote:
Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there
are two codepaths in pack-objects: with & without using bitmap
reachability index.
Sorry, I got distracted from reviewing these patches. I'll give them a
detailed look now and hopefully we can finalize the topic.
In want_object_in_pack() we care to start the checks from already found
pack, if we have one, this way determining the answer right away
in case neither --local nor --honour-pack-keep are active. In
particular, as p5310-pack-bitmaps.sh shows, we do not do harm to
served-with-bitmap clones performance-wise:
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.63(8.67+0.33) 9.47(8.55+0.28) -1.7%
5310.3: simulated clone 2.07(2.17+0.12) 2.03(2.14+0.12) -1.9%
5310.4: simulated fetch 0.78(1.03+0.02) 0.76(1.00+0.03) -2.6%
5310.6: partial bitmap 1.97(2.43+0.15) 1.92(2.36+0.14) -2.5%
with all differences strangely showing we are a bit faster now, but
probably all being within noise.
Good to know there is no regression. It is curious that there is a
slight _improvement_ across the board. Do we have an explanation for
that? It seems odd that noise would be so consistent.
And in the general case we care not to have duplicate
find_pack_entry_one(*found_pack) calls. Worst what can happen is we can
call want_found_object(*found_pack) -- newly introduced helper for
checking whether we want object -- twice, but since want_found_object()
is very lightweight it does not make any difference.
I had trouble parsing this. I think maybe:
In the general case we do not want to call find_pack_entry_one() more
than once, because it is expensive. This patch splits the loop in
want_object_in_pack() into two parts: finding the object and seeing if
it impacts our choice to include it in the pack. We may call the
inexpensive want_found_object() twice, but we will never call
find_pack_entry_one() if we do not need to.
+static int want_found_object(int exclude, struct packed_git *p)
+{
+ if (exclude)
+ return 1;
+ if (incremental)
+ return 0;
+
+ /*
+ * When asked to do --local (do not include an object that appears in a
+ * pack we borrow from elsewhere) or --honor-pack-keep (do not include
+ * an object that appears in a pack marked with .keep), finding a pack
+ * that matches the criteria is sufficient for us to decide to omit it.
+ * However, even if this pack does not satisfy the criteria, we need to
+ * make sure no copy of this object appears in _any_ pack that makes us
+ * to omit the object, so we need to check all the packs. Signal that by
+ * returning -1 to the caller.
+ */
+ if (!ignore_packed_keep &&
+ (!local || !have_non_local_packs))
+ return 1;
Hmm. The comment says "-1", but the return says "1". That is because the
comment is describing the return that happens at the end. :)
I wonder if the last sentence should be:
We can check here whether these options can possibly matter; if not,
we can return early from the function here. Otherwise, we signal "-1"
at the end to tell the caller that we do not know either way, and it
needs to check more packs.
- *found_pack = NULL;
- *found_offset = 0;
+ /*
+ * If we already know the pack object lives in, start checks from that
+ * pack - in the usual case when neither --local was given nor .keep files
+ * are present we will determine the answer right now.
+ */
+ if (*found_pack) {
+ want = want_found_object(exclude, *found_pack);
+ if (want != -1)
+ return want;
+ }
Looks correct. Though it is not really "start checks from..." anymore,
but rather "do a quick check to see if we can quit early, and otherwise
start the loop". That might be nitpicking, though.
I think technically we don't need to initialize found_offset here (it is
considered only if *found_pack is not NULL), but it doesn't hurt to make
our starting assumptions clear.
quoted hunk
@@ -1073,6 +1097,9 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1, if (have_duplicate_entry(sha1, 0, &index_pos)) return 0;+ if (!want_object_in_pack(sha1, 0, &pack, &offset))+ return 0;+
And this caller doesn't need to worry about initialization, because of
course it knows it has a pack/offset already. Good.
Tests look OK. I saw a few style nitpicks, but I think they are not even
against our style guide but more "I would have written it like this" and
are not even worth quibbling over.
So I think the code here is fine, and I just had a few minor complaints
on comment and commit message clarity.
-Peff
From: Jeff King <hidden> Date: 2016-08-19 01:12:59
On Tue, Aug 09, 2016 at 10:32:17PM +0300, Kirill Smelkov wrote:
Subject: Re: [PATCH 2/2 v7] pack-objects: use reachability bitmap index when
generating non-stdout pack
This is v7, but as I understand your numbering, it goes with v5 of patch
1/2 that I just reviewed (usually we just increment the version number
on the whole series and treat it as a unit, even if some patches didn't
change from version to version).
So we can teach pack-objects to use bitmap index for initial object
counting phase when generating resultant pack file too:
- if we care it is not activated under git-repack:
Do you mean "if we take care that it is not..." here?
(I think you might just be getting tripped up in the English idioms;
"care" means that we have a preference; "to take care" means that we are
being careful).
- if we know bitmap index generation is not enabled for resultant pack:
Current code has singleton bitmap_git so cannot work simultaneously
with two bitmap indices.
Minor English fixes:
The current code has a singleton bitmap_git, so it cannot work
simultaneously with two bitmap indices.
- if we keep pack reuse enabled still only for "send-to-stdout" case:
Because on pack reuse raw entries are directly written out to destination
pack by write_reused_pack() bypassing needed for pack index generation
bookkeeping done by regular codepath in write_one() and friends.
Ditto on English:
On pack reuse raw entries are directly written out to the destination
pack by write_reused_pack(), bypassing the need for pack index
generation bookkeeping done by the regular code path in write_one()
and friends.
I think this is missing the implication. Why wouldn't we want to reuse
in this case? Certainly we don't when doing a "careful" on-disk repack.
I suspect the answer is that we cannot write a ".idx" off of the result
of write_reused_pack(), and write-to-disk always includes the .idx.
Can we turn this into a link to public-inbox? We have just been bit by
all of our old links to gmane dying, and they cannot easily be replaced
because they use a gmane-specific article number. public-inbox URLs use
message-ids, which should be usable for other archives if public-inbox
goes away.
@@ -196,6 +196,18 @@ test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '!has_anypackbitmap.objects3b.objects'+test_expect_success'pack-objects to file can use bitmap''+# make sure we still have 1 bitmap index from previous tests+ls.git/objects/pack/|grepbitmap>output&&+test_line_count=1output&&+# verify equivalent packs are generated with/without using bitmap index+packasha1=$(gitpack-objects--no-use-bitmap-index--allpacka</dev/null)&&+packbsha1=$(gitpack-objects--use-bitmap-index--allpackb</dev/null)&&+list_packed_objects<packa-$packasha1.idx>packa.objects&&+list_packed_objects<packb-$packbsha1.idx>packb.objects&&+test_cmppacka.objectspackb.objects+'
Of course we can't know if bitmaps were actually used, or if they were
turned off under the hood. But at least this exercises the code a bit.
You could possibly add a perf test which shows off the improvement, but
I don't think it's strictly necessary.
-Peff
On Thu, Aug 18, 2016 at 01:52:22PM -0400, Jeff King wrote:
On Tue, Aug 09, 2016 at 10:31:43PM +0300, Kirill Smelkov wrote:
quoted
Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there
are two codepaths in pack-objects: with & without using bitmap
reachability index.
Sorry, I got distracted from reviewing these patches. I'll give them a
detailed look now and hopefully we can finalize the topic.
Jeff, thanks for feedback. On my side I'm sorry for the delay because I
was travelling and only recently got back to work.
quoted
In want_object_in_pack() we care to start the checks from already found
pack, if we have one, this way determining the answer right away
in case neither --local nor --honour-pack-keep are active. In
particular, as p5310-pack-bitmaps.sh shows, we do not do harm to
served-with-bitmap clones performance-wise:
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.63(8.67+0.33) 9.47(8.55+0.28) -1.7%
5310.3: simulated clone 2.07(2.17+0.12) 2.03(2.14+0.12) -1.9%
5310.4: simulated fetch 0.78(1.03+0.02) 0.76(1.00+0.03) -2.6%
5310.6: partial bitmap 1.97(2.43+0.15) 1.92(2.36+0.14) -2.5%
with all differences strangely showing we are a bit faster now, but
probably all being within noise.
Good to know there is no regression. It is curious that there is a
slight _improvement_ across the board. Do we have an explanation for
that? It seems odd that noise would be so consistent.
Yes, I too thought it and it turned out to be t/perf/run does not copy
config.mak.autogen & friends to build/ and I'm using autoconf with
CFLAGS="-march=native -O3 ..."
Junio, I could not resist to the following:
---- 8< ----
From: Kirill Smelkov <redacted>
Subject: [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends
to build area
Otherwise for people who use autotools-based configure in main worktree,
the performance testing results will be inconsistent as work and build
trees could be using e.g. different optimization levels.
See e.g.
http://public-inbox.org/git/20160818175222.bmm3ivjheokf2qzl@sigill.intra.peff.net/
for example.
NOTE config.status has to be copied because otherwise without it the build
would want to run reconfigure this way loosing just copied config.mak.autogen.
Signed-off-by: Kirill Smelkov <redacted>
---
t/perf/run | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
2.9.2.701.gf965a18.dirty
---- 8< ----
With corrected t/perf/run the timings are more realistic - e.g. 3
consecutive runs of `./run 56dfeb62 . ./p5310-pack-bitmaps.sh`:
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.08(8.20+0.25) 9.09(8.14+0.32) +0.1%
5310.3: simulated clone 1.92(2.12+0.08) 1.93(2.12+0.09) +0.5%
5310.4: simulated fetch 0.82(1.07+0.04) 0.82(1.06+0.04) +0.0%
5310.6: partial bitmap 1.96(2.42+0.13) 1.95(2.40+0.15) -0.5%
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.11(8.16+0.32) 9.11(8.19+0.28) +0.0%
5310.3: simulated clone 1.93(2.14+0.07) 1.92(2.11+0.10) -0.5%
5310.4: simulated fetch 0.82(1.06+0.04) 0.82(1.04+0.05) +0.0%
5310.6: partial bitmap 1.95(2.38+0.16) 1.94(2.39+0.14) -0.5%
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.13(8.17+0.31) 9.07(8.13+0.28) -0.7%
5310.3: simulated clone 1.92(2.13+0.07) 1.91(2.12+0.06) -0.5%
5310.4: simulated fetch 0.82(1.08+0.03) 0.82(1.08+0.03) +0.0%
5310.6: partial bitmap 1.96(2.43+0.14) 1.96(2.42+0.14) +0.0%
> > And in the general case we care not to have duplicate
> > find_pack_entry_one(*found_pack) calls. Worst what can happen is we can
> > call want_found_object(*found_pack) -- newly introduced helper for
> > checking whether we want object -- twice, but since want_found_object()
> > is very lightweight it does not make any difference.
>
> I had trouble parsing this. I think maybe:
>
> In the general case we do not want to call find_pack_entry_one() more
> than once, because it is expensive. This patch splits the loop in
> want_object_in_pack() into two parts: finding the object and seeing if
> it impacts our choice to include it in the pack. We may call the
> inexpensive want_found_object() twice, but we will never call
> find_pack_entry_one() if we do not need to.
Ok, thanks for the advice.
>
> > +static int want_found_object(int exclude, struct packed_git *p)
> > +{
> > + if (exclude)
> > + return 1;
> > + if (incremental)
> > + return 0;
> > +
> > + /*
> > + * When asked to do --local (do not include an object that appears in a
> > + * pack we borrow from elsewhere) or --honor-pack-keep (do not include
> > + * an object that appears in a pack marked with .keep), finding a pack
> > + * that matches the criteria is sufficient for us to decide to omit it.
> > + * However, even if this pack does not satisfy the criteria, we need to
> > + * make sure no copy of this object appears in _any_ pack that makes us
> > + * to omit the object, so we need to check all the packs. Signal that by
> > + * returning -1 to the caller.
> > + */
> > + if (!ignore_packed_keep &&
> > + (!local || !have_non_local_packs))
> > + return 1;
>
> Hmm. The comment says "-1", but the return says "1". That is because the
> comment is describing the return that happens at the end. :)
>
> I wonder if the last sentence should be:
>
> We can check here whether these options can possibly matter; if not,
> we can return early from the function here. Otherwise, we signal "-1"
> at the end to tell the caller that we do not know either way, and it
> needs to check more packs.
Thanks for the catch and hint. I've changed it to the following:
We can however first check whether these options can possible matter;
if they do not matter we know we want the object in generated pack.
Otherwise, we signal "-1" at the end to tell the caller that we do
not know either way, and it needs to check more packs.
full version:
/*
* When asked to do --local (do not include an object that appears in a
* pack we borrow from elsewhere) or --honor-pack-keep (do not include
* an object that appears in a pack marked with .keep), finding a pack
* that matches the criteria is sufficient for us to decide to omit it.
* However, even if this pack does not satisfy the criteria, we need to
* make sure no copy of this object appears in _any_ pack that makes us
* to omit the object, so we need to check all the packs.
*
* We can however first check whether these options can possible matter;
* if they do not matter we know we want the object in generated pack.
* Otherwise, we signal "-1" at the end to tell the caller that we do
* not know either way, and it needs to check more packs.
*/
Hope it is ok.
> > - *found_pack = NULL;
> > - *found_offset = 0;
> > + /*
> > + * If we already know the pack object lives in, start checks from that
> > + * pack - in the usual case when neither --local was given nor .keep files
> > + * are present we will determine the answer right now.
> > + */
> > + if (*found_pack) {
> > + want = want_found_object(exclude, *found_pack);
> > + if (want != -1)
> > + return want;
> > + }
>
> Looks correct. Though it is not really "start checks from..." anymore,
> but rather "do a quick check to see if we can quit early, and otherwise
> start the loop". That might be nitpicking, though.
I see. Your version is ok, but to me 'start checks from ...' is a bit
more natural and explaining (yes, all subjective and depending on
taste), so if possible I'd prefer to leave it as is.
>
> > for (p = packed_git; p; p = p->next) {
> > - off_t offset = find_pack_entry_one(sha1, p);
> > + off_t offset;
> > +
> > + if (p == *found_pack)
> > + offset = *found_offset;
> > + else
> > + offset = find_pack_entry_one(sha1, p);
> > +
>
> This hunk will conflict with the MRU optimizations in 'next', but I
> think the resolution should be pretty trivial.
Yes.
> > static int add_object_entry(const unsigned char *sha1, enum object_type type,
> > const char *name, int exclude)
> > {
> > - struct packed_git *found_pack;
> > - off_t found_offset;
> > + struct packed_git *found_pack = NULL;
> > + off_t found_offset = 0;
>
> I think technically we don't need to initialize found_offset here (it is
> considered only if *found_pack is not NULL), but it doesn't hurt to make
> our starting assumptions clear.
Yes, found_pack != NULL is indicator whether we have found_pack /
found_offset info, but it makes it much clear and defending from
mistakes to set both found_{pack,offset} into known initial state.
> > @@ -1073,6 +1097,9 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,
> > if (have_duplicate_entry(sha1, 0, &index_pos))
> > return 0;
> >
> > + if (!want_object_in_pack(sha1, 0, &pack, &offset))
> > + return 0;
> > +
>
> And this caller doesn't need to worry about initialization, because of
> course it knows it has a pack/offset already. Good.
Yes, we have this info from bitmap walker calling us.
> > diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> > index 3893afd..a278d30 100755
> > --- a/t/t5310-pack-bitmaps.sh
> > +++ b/t/t5310-pack-bitmaps.sh
>
> Tests look OK. I saw a few style nitpicks, but I think they are not even
> against our style guide but more "I would have written it like this" and
> are not even worth quibbling over.
>
> So I think the code here is fine, and I just had a few minor complaints
> on comment and commit message clarity.
Thanks for feedback. Yes tastes can differ but your comments regarding
commit message and want_found_object() were objectively (imho) worth it
and there I've made the adjustments.
Please expect updated patch to be send as reply to this mail.
Thanks again for feedback,
Kirill
On Thu, Aug 18, 2016 at 02:06:15PM -0400, Jeff King wrote:
On Tue, Aug 09, 2016 at 10:32:17PM +0300, Kirill Smelkov wrote:
quoted
Subject: Re: [PATCH 2/2 v7] pack-objects: use reachability bitmap index when
generating non-stdout pack
This is v7, but as I understand your numbering, it goes with v5 of patch
1/2 that I just reviewed (usually we just increment the version number
on the whole series and treat it as a unit, even if some patches didn't
change from version to version).
The reason those patches are having their own numbers is that they are
orthogonal to each other and can be applied / rejected independently.
Since I though Junio might want to pick them up as separate topics they
were versioned separately.
But ok, since now we have them considered both together, their next
versions posted will be uniform v8.
quoted
So we can teach pack-objects to use bitmap index for initial object
counting phase when generating resultant pack file too:
- if we care it is not activated under git-repack:
Do you mean "if we take care that it is not..." here?
(I think you might just be getting tripped up in the English idioms;
"care" means that we have a preference; "to take care" means that we are
being careful).
Ok, I've might have been tripped and thanks for the catch up. I've changed to
"if we take care to not let it be activated under git-repack"
quoted
- if we know bitmap index generation is not enabled for resultant pack:
Current code has singleton bitmap_git so cannot work simultaneously
with two bitmap indices.
Minor English fixes:
The current code has a singleton bitmap_git, so it cannot work
simultaneously with two bitmap indices.
ok.
quoted
- if we keep pack reuse enabled still only for "send-to-stdout" case:
Because on pack reuse raw entries are directly written out to destination
pack by write_reused_pack() bypassing needed for pack index generation
bookkeeping done by regular codepath in write_one() and friends.
Ditto on English:
On pack reuse raw entries are directly written out to the destination
pack by write_reused_pack(), bypassing the need for pack index
generation bookkeeping done by the regular code path in write_one()
and friends.
I think this is missing the implication. Why wouldn't we want to reuse
in this case? Certainly we don't when doing a "careful" on-disk repack.
I suspect the answer is that we cannot write a ".idx" off of the result
of write_reused_pack(), and write-to-disk always includes the .idx.
Yes, mentioning pack-to-file needs to generate .idx makes it more clear
and thanks for pointing this out. I've changed this item to the
following (picking some of your English corrections):
- if we keep pack reuse enabled still only for "send-to-stdout" case:
Because pack-to-file needs to generate index for destination pack, and
currently on pack reuse raw entries are directly written out to the
destination pack by write_reused_pack(), bypassing needed for pack index
generation bookkeeping done by regular codepath in write_one() and
friends.
( In the future we might teach pack-reuse code about cases when index
also needs to be generated for resultant pack and remove
pack-reuse-only-for-stdout limitation )
Hope it is ok.
Can we turn this into a link to public-inbox? We have just been bit by
all of our old links to gmane dying, and they cannot easily be replaced
because they use a gmane-specific article number. public-inbox URLs use
message-ids, which should be usable for other archives if public-inbox
goes away.
@@ -196,6 +196,18 @@ test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '!has_anypackbitmap.objects3b.objects'+test_expect_success'pack-objects to file can use bitmap''+# make sure we still have 1 bitmap index from previous tests+ls.git/objects/pack/|grepbitmap>output&&+test_line_count=1output&&+# verify equivalent packs are generated with/without using bitmap index+packasha1=$(gitpack-objects--no-use-bitmap-index--allpacka</dev/null)&&+packbsha1=$(gitpack-objects--use-bitmap-index--allpackb</dev/null)&&+list_packed_objects<packa-$packasha1.idx>packa.objects&&+list_packed_objects<packb-$packbsha1.idx>packb.objects&&+test_cmppacka.objectspackb.objects+'
Of course we can't know if bitmaps were actually used, or if they were
turned off under the hood. But at least this exercises the code a bit.
Yes, I was thinking how to know the bitmap codepath was actually active,
and without adding debugging points there is no way (at least I could
not find it).
You could possibly add a perf test which shows off the improvement, but
I don't think it's strictly necessary.
@@ -32,6 +32,14 @@ test_perf 'simulated fetch' '}|gitpack-objects--revs--stdout>/dev/null'+test_perf'pack to file''+gitpack-objects--allpack1</dev/null>/dev/null+'++test_perf'pack to file (bitmap)''+gitpack-objects--use-bitmap-index--allpack1b</dev/null>/dev/null+'+ test_expect_success'create partial bitmap state''# pick a commit to represent the repo tip in the pastcutoff=$(gitrev-listHEAD~100-1)&&
Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there
are two codepaths in pack-objects: with & without using bitmap
reachability index.
However add_object_entry_from_bitmap(), despite its non-bitmapped
counterpart add_object_entry(), in no way does check for whether --local
or --honor-pack-keep or --incremental should be respected. In
non-bitmapped codepath this is handled in want_object_in_pack(), but
bitmapped codepath has simply no such checking at all.
The bitmapped codepath however was allowing to pass in all those options
and with bitmap indices still being used under such conditions -
potentially giving wrong output (e.g. including objects from non-local or
.keep'ed pack).
We can easily fix this by noting the following: when an object comes to
add_object_entry_from_bitmap() it can come for two reasons:
1. entries coming from main pack covered by bitmap index, and
2. object coming from, possibly alternate, loose or other packs.
"2" can be already handled by want_object_in_pack() and to cover
"1" we can teach want_object_in_pack() to expect that *found_pack can be
non-NULL, meaning calling client already found object's pack entry.
In want_object_in_pack() we care to start the checks from already found
pack, if we have one, this way determining the answer right away
in case neither --local nor --honour-pack-keep are active. In
particular, as p5310-pack-bitmaps.sh shows (3 consecutive runs), we do
not do harm to served-with-bitmap clones performance-wise:
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.08(8.20+0.25) 9.09(8.14+0.32) +0.1%
5310.3: simulated clone 1.92(2.12+0.08) 1.93(2.12+0.09) +0.5%
5310.4: simulated fetch 0.82(1.07+0.04) 0.82(1.06+0.04) +0.0%
5310.6: partial bitmap 1.96(2.42+0.13) 1.95(2.40+0.15) -0.5%
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.11(8.16+0.32) 9.11(8.19+0.28) +0.0%
5310.3: simulated clone 1.93(2.14+0.07) 1.92(2.11+0.10) -0.5%
5310.4: simulated fetch 0.82(1.06+0.04) 0.82(1.04+0.05) +0.0%
5310.6: partial bitmap 1.95(2.38+0.16) 1.94(2.39+0.14) -0.5%
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.13(8.17+0.31) 9.07(8.13+0.28) -0.7%
5310.3: simulated clone 1.92(2.13+0.07) 1.91(2.12+0.06) -0.5%
5310.4: simulated fetch 0.82(1.08+0.03) 0.82(1.08+0.03) +0.0%
5310.6: partial bitmap 1.96(2.43+0.14) 1.96(2.42+0.14) +0.0%
with delta timings showing they are all within noise from run to run.
In the general case we do not want to call find_pack_entry_one() more than
once, because it is expensive. This patch splits the loop in
want_object_in_pack() into two parts: finding the object and seeing if it
impacts our choice to include it in the pack. We may call the inexpensive
want_found_object() twice, but we will never call find_pack_entry_one() if we
do not need to.
I appreciate help and discussing this change with Junio C Hamano and
Jeff King.
Signed-off-by: Kirill Smelkov <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
builtin/pack-objects.c | 97 ++++++++++++++++++++++++++++++++-----------------
t/t5310-pack-bitmaps.sh | 92 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 156 insertions(+), 33 deletions(-)
@@ -944,13 +944,48 @@ static int have_duplicate_entry(const unsigned char *sha1,return1;}+staticintwant_found_object(intexclude,structpacked_git*p)+{+if(exclude)+return1;+if(incremental)+return0;++/*+*Whenaskedtodo--local(donotincludeanobjectthatappearsina+*packweborrowfromelsewhere)or--honor-pack-keep(donotinclude+*anobjectthatappearsinapackmarkedwith.keep),findingapack+*thatmatchesthecriteriaissufficientforustodecidetoomitit.+*However,evenifthispackdoesnotsatisfythecriteria,weneedto+*makesurenocopyofthisobjectappearsin_any_packthatmakesus+*toomittheobject,soweneedtocheckallthepacks.+*+*Wecanhoweverfirstcheckwhethertheseoptionscanpossiblematter;+*iftheydonotmatterweknowwewanttheobjectingeneratedpack.+*Otherwise,wesignal"-1"attheendtotellthecallerthatwedo+*notknoweitherway,anditneedstocheckmorepacks.+*/+if(!ignore_packed_keep&&+(!local||!have_non_local_packs))+return1;++if(local&&!p->pack_local)+return0;+if(ignore_packed_keep&&p->pack_local&&p->pack_keep)+return0;++/* we don't know yet; keep looking for more packs */+return-1;+}+/**Checkwhetherwewanttheobjectinthepack(e.g.,wedonotwant*objectsfoundinnon-localstoresifthe"--local"optionwasused).*-*Asasideeffectofthischeck,wewillfindthepackedversionofthis-*object,ifany.Wethereforepassoutthepackinformationtoavoidhaving-*tolookitupagainlater.+*Ifthecalleralreadyknowsanexistingpackitwantstotaketheobject+*from,thatispassedin*found_packand*found_offset;otherwisethis+*functionfindsifthereisanypackthathastheobjectandreturnsthepack+*anditsoffsetinthesevariables.*/staticintwant_object_in_pack(constunsignedchar*sha1,intexclude,
@@ -958,15 +993,30 @@ static int want_object_in_pack(const unsigned char *sha1,off_t*found_offset){structpacked_git*p;+intwant;if(!exclude&&local&&has_loose_object_nonlocal(sha1))return0;-*found_pack=NULL;-*found_offset=0;+/*+*Ifwealreadyknowthepackobjectlivesin,startchecksfromthat+*pack-intheusualcasewhenneither--localwasgivennor.keepfiles+*arepresentwewilldeterminetheanswerrightnow.+*/+if(*found_pack){+want=want_found_object(exclude,*found_pack);+if(want!=-1)+returnwant;+}for(p=packed_git;p;p=p->next){-off_toffset=find_pack_entry_one(sha1,p);+off_toffset;++if(p==*found_pack)+offset=*found_offset;+else+offset=find_pack_entry_one(sha1,p);+if(offset){if(!*found_pack){if(!is_pack_valid(p))
@@ -974,31 +1024,9 @@ static int want_object_in_pack(const unsigned char *sha1,*found_offset=offset;*found_pack=p;}-if(exclude)-return1;-if(incremental)-return0;--/*-*Whenaskedtodo--local(donotincludean-*objectthatappearsinapackweborrow-*fromelsewhere)or--honor-pack-keep(donot-*includeanobjectthatappearsinapackmarked-*with.keep),weneedtomakesurenocopyofthis-*objectcomefromin_any_packthatcausesusto-*omitit,andneedtocompletethisloop.When-*neitheroptionisineffect,weknowtheobject-*wejustfoundisgoingtobepacked,sobreak-*outofthelooptoreturn1now.-*/-if(!ignore_packed_keep&&-(!local||!have_non_local_packs))-break;--if(local&&!p->pack_local)-return0;-if(ignore_packed_keep&&p->pack_local&&p->pack_keep)-return0;+want=want_found_object(exclude,p);+if(want!=-1)+returnwant;}}
@@ -7,6 +7,18 @@ objpath () {echo".git/objects/$(echo"$1"|sed-e's|\(..\)|\1/|')"}+# show objects present in pack ($1 should be associated *.idx)+list_packed_objects(){+gitshow-index<"$1"|cut-d' '-f2+}++# has_any pattern-file content-file+# tests whether content-file has any entry from pattern-file with entries being+# whole lines.+has_any(){+grep-Ff"$1""$2"+}+ test_expect_success'setup repo with moderate-sized history''foriin$(test_seq110);dotest_commit$i
Starting from 6b8fda2d (pack-objects: use bitmaps when packing objects)
if a repository has bitmap index, pack-objects can nicely speedup
"Counting objects" graph traversal phase. That however was done only for
case when resultant pack is sent to stdout, not written into a file.
The reason here is for on-disk repack by default we want:
- to produce good pack (with bitmap index not-yet-packed objects are
emitted to pack in suboptimal order).
- to use more robust pack-generation codepath (avoiding possible
bugs in bitmap code and possible bitmap index corruption).
Jeff King further explains:
The reason for this split is that pack-objects tries to determine how
"careful" it should be based on whether we are packing to disk or to
stdout. Packing to disk implies "git repack", and that we will likely
delete the old packs after finishing. We want to be more careful (so
as not to carry forward a corruption, and to generate a more optimal
pack), and we presumably run less frequently and can afford extra CPU.
Whereas packing to stdout implies serving a remote via "git fetch" or
"git push". This happens more frequently (e.g., a server handling many
fetching clients), and we assume the receiving end takes more
responsibility for verifying the data.
But this isn't always the case. One might want to generate on-disk
packfiles for a specialized object transfer. Just using "--stdout" and
writing to a file is not optimal, as it will not generate the matching
pack index.
So it would be useful to have some way of overriding this heuristic:
to tell pack-objects that even though it should generate on-disk
files, it is still OK to use the reachability bitmaps to do the
traversal.
So we can teach pack-objects to use bitmap index for initial object
counting phase when generating resultant pack file too:
- if we take care to not let it be activated under git-repack:
See above about repack robustness and not forward-carrying corruption.
- if we know bitmap index generation is not enabled for resultant pack:
The current code has singleton bitmap_git, so it cannot work
simultaneously with two bitmap indices.
We also want to avoid (at least with current implementation)
generating bitmaps off of bitmaps. The reason here is: when generating
a pack, not-yet-packed objects will be emitted into pack in
suboptimal order and added to tail of the bitmap as "extended entries".
When the resultant pack + some new objects in associated repository
are in turn used to generate another pack with bitmap, the situation
repeats: new objects are again not emitted optimally and just added to
bitmap tail - not in recency order.
So the pack badness can grow over time when at each step we have
bitmapped pack + some other objects. That's why we want to avoid
generating bitmaps off of bitmaps, not to let pack badness grow.
- if we keep pack reuse enabled still only for "send-to-stdout" case:
Because pack-to-file needs to generate index for destination pack, and
currently on pack reuse raw entries are directly written out to the
destination pack by write_reused_pack(), bypassing needed for pack index
generation bookkeeping done by regular codepath in write_one() and
friends.
( In the future we might teach pack-reuse code about cases when index
also needs to be generated for resultant pack and remove
pack-reuse-only-for-stdout limitation )
This way for pack-objects -> file we get nice speedup:
erp5.git[1] (~230MB) extracted from ~ 5GB lab.nexedi.com backup
repository managed by git-backup[2] via
time echo 0186ac99 | git pack-objects --revs erp5pack
before: 37.2s
after: 26.2s
And for `git repack -adb` packed git.git
time echo 5c589a73 | git pack-objects --revs gitpack
before: 7.1s
after: 3.6s
i.e. it can be 30% - 50% speedup for pack extraction.
git-backup extracts many packs on repositories restoration. That was my
initial motivation for the patch.
[1] https://lab.nexedi.com/nexedi/erp5
[2] https://lab.nexedi.com/kirr/git-backup
NOTE
Jeff also suggests that pack.useBitmaps was probably a mistake to
introduce originally. This way we are not adding another config point,
but instead just always default to-file pack-objects not to use bitmap
index: Tools which need to generate on-disk packs with using bitmap, can
pass --use-bitmap-index explicitly. And git-repack does never pass
--use-bitmap-index, so this way we can be sure regular on-disk repacking
remains robust.
NOTE2
`git pack-objects --stdout >file.pack` + `git index-pack file.pack` is much slower
than `git pack-objects file.pack`. Extracting erp5.git pack from
lab.nexedi.com backup repository:
$ time echo 0186ac99 | git pack-objects --stdout --revs >erp5pack-stdout.pack
real 0m22.309s
user 0m21.148s
sys 0m0.932s
$ time git index-pack erp5pack-stdout.pack
real 0m50.873s <-- more than 2 times slower than time to generate pack itself!
user 0m49.300s
sys 0m1.360s
So the time for
`pack-object --stdout >file.pack` + `index-pack file.pack` is 72s,
while
`pack-objects file.pack` which does both pack and index is 27s.
And even
`pack-objects --no-use-bitmap-index file.pack` is 37s.
Jeff explains:
The packfile does not carry the sha1 of the objects. A receiving
index-pack has to compute them itself, including inflating and applying
all of the deltas.
that's why for `git-backup restore` we want to teach `git pack-objects
file.pack` to use bitmaps instead of using `git pack-objects --stdout
file.pack` + `git index-pack file.pack`.
NOTE3
The speedup is now tracked via t/perf/p5310-pack-bitmaps.sh
Test 56dfeb62 this tree
--------------------------------------------------------------------------------
5310.2: repack to disk 8.98(8.05+0.29) 9.05(8.08+0.33) +0.8%
5310.3: simulated clone 2.02(2.27+0.09) 2.01(2.25+0.08) -0.5%
5310.4: simulated fetch 0.81(1.07+0.02) 0.81(1.05+0.04) +0.0%
5310.5: pack to file 7.58(7.04+0.28) 7.60(7.04+0.30) +0.3%
5310.6: pack to file (bitmap) 7.55(7.02+0.28) 3.25(2.82+0.18) -57.0%
5310.8: clone (partial bitmap) 1.83(2.26+0.12) 1.82(2.22+0.14) -0.5%
5310.9: pack to file (partial bitmap) 6.86(6.58+0.30) 2.87(2.74+0.20) -58.2%
More context:
http://marc.info/?t=146792101400001&r=1&w=2http://public-inbox.org/git/20160707190917.20011-1-kirr@nexedi.com/T/#t
Cc: Vicent Marti <redacted>
Helped-by: Jeff King [off-list ref]
Signed-off-by: Kirill Smelkov <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
builtin/pack-objects.c | 31 ++++++++++++++++++++++++-------
t/perf/p5310-pack-bitmaps.sh | 14 +++++++++++++-
t/t5310-pack-bitmaps.sh | 12 ++++++++++++
3 files changed, 49 insertions(+), 8 deletions(-)
@@ -2822,7 +2823,23 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)if(!rev_list_all||!rev_list_reflog||!rev_list_index)unpack_unreachable_expiration=0;-if(!use_internal_rev_list||!pack_to_stdout||is_repository_shallow())+/*+*"soft"reasonsnottousebitmaps-foron-diskrepackbydefaultwewant+*+*-toproducegoodpack(withbitmapindexnot-yet-packedobjectsare+*packedinsuboptimalorder).+*+*-tousemorerobustpack-generationcodepath(avoidingpossible+*bugsinbitmapcodeandpossiblebitmapindexcorruption).+*/+if(!pack_to_stdout)+use_bitmap_index_default=0;++if(use_bitmap_index<0)+use_bitmap_index=use_bitmap_index_default;++/* "hard" reasons not to use bitmaps; these just won't work at all */+if(!use_internal_rev_list||(!pack_to_stdout&&write_bitmap_index)||is_repository_shallow())use_bitmap_index=0;if(pack_to_stdout||!rev_list_all)
@@ -32,6 +32,14 @@ test_perf 'simulated fetch' '}|gitpack-objects--revs--stdout>/dev/null'+test_perf'pack to file''+gitpack-objects--allpack1</dev/null>/dev/null+'++test_perf'pack to file (bitmap)''+gitpack-objects--use-bitmap-index--allpack1b</dev/null>/dev/null+'+ test_expect_success'create partial bitmap state''# pick a commit to represent the repo tip in the pastcutoff=$(gitrev-listHEAD~100-1)&&
@@ -196,6 +196,18 @@ test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '!has_anypackbitmap.objects3b.objects'+test_expect_success'pack-objects to file can use bitmap''+# make sure we still have 1 bitmap index from previous tests+ls.git/objects/pack/|grepbitmap>output&&+test_line_count=1output&&+# verify equivalent packs are generated with/without using bitmap index+packasha1=$(gitpack-objects--no-use-bitmap-index--allpacka</dev/null)&&+packbsha1=$(gitpack-objects--use-bitmap-index--allpackb</dev/null)&&+list_packed_objects<packa-$packasha1.idx>packa.objects&&+list_packed_objects<packb-$packbsha1.idx>packb.objects&&+test_cmppacka.objectspackb.objects+'+ test_expect_success'full repack, reusing previous bitmaps''gitrepack-ad&&ls.git/objects/pack/|grepbitmap>output&&
Otherwise for people who use autotools-based configure in main worktree,
the performance testing results will be inconsistent as work and build
trees could be using e.g. different optimization levels.
See e.g.
http://public-inbox.org/git/20160818175222.bmm3ivjheokf2qzl@sigill.intra.peff.net/
for example.
NOTE config.status has to be copied because otherwise without it the build
would want to run reconfigure this way loosing just copied config.mak.autogen.
Signed-off-by: Kirill Smelkov <redacted>
---
( Resending as separate patch-mail, just in case )
t/perf/run | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)