| Message ID | a2d0af562a5d0a4052c94f87c1d71639ff0b87f2.1705431816.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pack-objects: enable multi-pack reuse via feature.experimental | expand |
On Tue, Jan 16, 2024 at 02:03:47PM -0500, Taylor Blau wrote: > Now that multi-pack reuse is supported, enable it via the > feature.experimental configuration in addition to the classic > `pack.allowPackReuse`. > > This will allow more users to experiment with the new behavior who might > not otherwise be aware of the existing `pack.allowPackReuse` > configuration option. > > The enum with values NO_PACK_REUSE, SINGLE_PACK_REUSE, and > MULTI_PACK_REUSE is defined statically in builtin/pack-objects.c's > compilation unit. We could hoist that enum into a scope visible from the > repository_settings struct, and then use that enum value in > pack-objects. Instead, define a single int that indicates what > pack-objects's default value should be to avoid additional unnecessary > code movement. > > Though `feature.experimental` implies `pack.allowPackReuse=multi`, this > can still be overridden by explicitly setting the latter configuration > to either "single" or "false". Tests covering all of these cases are > showin t5332. I do not mind adding more configs to `feature.experimental` because it is the best mechanism we have for a staged rollout of features. It is not ideal by any means as we have no way to tell how many people enable this, or whether they hit any bugs. But we do not really have any alternatives. But one thing I would like to see is to have a clear plan for how experimental features become stable. It's not a huge problem (yet) because there are only two experimental features. One of them ("pack.useBitmapBoundaryTraversal=true") was recently added by you via b0afdce5da (pack-bitmap.c: use commit boundary during bitmap traversal, 2023-05-08), which is perfectly fine. But the other one ("fetch.negotiationAlgorithm=skipping") has been added has been added via b5651a2092 (experimental: default to fetch.writeCommitGraph=false, 2020-07-06), so it's been experimental for ~3.5 years by now. I would like to avoid cases like this by laying out a plan for when experimental features become the new default. It could be as simple as "Let's wait N releases and then mark it stable". But having something and documenting such a plan in our code makes it a lot more actionable to promote those features to become stable eventually. I know that this is not in any way specific to your patch, but I thought this was a good opportunity to start this discussion. If we can agree on my opinion then it would be great to add a comment to the experimental feature to explain such an exit criterion. Other than that this patch looks good to me, thanks! Patrick
On Wed, Jan 17, 2024 at 08:32:15AM +0100, Patrick Steinhardt wrote: > I would like to avoid cases like this by laying out a plan for when > experimental features become the new default. It could be as simple as > "Let's wait N releases and then mark it stable". But having something > and documenting such a plan in our code makes it a lot more actionable > to promote those features to become stable eventually. Fair point. I think that these have been mostly ad-hoc. Unfortunately, when folks leave the project or change their attention to working on a different feature, things that were left in feature.experimental may be forgotten about. When this inevitably happens, it would be nice to have a written record (either in the repository, or here on the mailing list) so that other folks can take up the mantle and graduate those feature(s) as appropriate. > I know that this is not in any way specific to your patch, but I thought > this was a good opportunity to start this discussion. If we can agree on > my opinion then it would be great to add a comment to the experimental > feature to explain such an exit criterion. I don't have a ton to add here for a graduation plan other than it would be nice to enable it eventually, likely after a few releases without any show-stopping bug reports. > Other than that this patch looks good to me, thanks! Thanks again for the review! Thanks, Taylor
diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt index bf9546fca4..f061b64b74 100644 --- a/Documentation/config/feature.txt +++ b/Documentation/config/feature.txt @@ -17,6 +17,9 @@ skipping more commits at a time, reducing the number of round trips. + * `pack.useBitmapBoundaryTraversal=true` may improve bitmap traversal times by walking fewer objects. ++ +* `pack.allowPackReuse=multi` may improve the time it takes to create a pack by +reusing objects from multiple packs instead of just one. feature.manyFiles:: Enable config options that optimize for repos with many files in the diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index d8c2128a97..329aeac804 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4396,6 +4396,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) prepare_repo_settings(the_repository); if (sparse < 0) sparse = the_repository->settings.pack_use_sparse; + if (the_repository->settings.pack_use_multi_pack_reuse) + allow_pack_reuse = MULTI_PACK_REUSE; } reset_pack_idx_option(&pack_idx_opts); diff --git a/repo-settings.c b/repo-settings.c index 30cd478762..a0b590bc6c 100644 --- a/repo-settings.c +++ b/repo-settings.c @@ -43,6 +43,7 @@ void prepare_repo_settings(struct repository *r) if (experimental) { r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING; r->settings.pack_use_bitmap_boundary_traversal = 1; + r->settings.pack_use_multi_pack_reuse = 1; } if (manyfiles) { r->settings.index_version = 4; diff --git a/repository.h b/repository.h index 5f18486f64..b92881b0a3 100644 --- a/repository.h +++ b/repository.h @@ -36,6 +36,7 @@ struct repo_settings { int sparse_index; int pack_read_reverse_index; int pack_use_bitmap_boundary_traversal; + int pack_use_multi_pack_reuse; /* * Does this repository have core.useReplaceRefs=true (on by diff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh index b53e821bc0..ccc8735db6 100755 --- a/t/t5332-multi-pack-reuse.sh +++ b/t/t5332-multi-pack-reuse.sh @@ -57,6 +57,22 @@ test_expect_success 'preferred pack is reused for single-pack reuse' ' test_pack_objects_reused_all 3 1 ' +test_expect_success 'multi-pack reuse is disabled by default' ' + test_pack_objects_reused_all 3 1 +' + +test_expect_success 'feature.experimental implies multi-pack reuse' ' + test_config feature.experimental true && + + test_pack_objects_reused_all 6 2 +' + +test_expect_success 'multi-pack reuse can be disabled with feature.experimental' ' + test_config feature.experimental true && + test_config pack.allowPackReuse single && + + test_pack_objects_reused_all 3 1 +' test_expect_success 'enable multi-pack reuse' ' git config pack.allowPackReuse multi
Now that multi-pack reuse is supported, enable it via the feature.experimental configuration in addition to the classic `pack.allowPackReuse`. This will allow more users to experiment with the new behavior who might not otherwise be aware of the existing `pack.allowPackReuse` configuration option. The enum with values NO_PACK_REUSE, SINGLE_PACK_REUSE, and MULTI_PACK_REUSE is defined statically in builtin/pack-objects.c's compilation unit. We could hoist that enum into a scope visible from the repository_settings struct, and then use that enum value in pack-objects. Instead, define a single int that indicates what pack-objects's default value should be to avoid additional unnecessary code movement. Though `feature.experimental` implies `pack.allowPackReuse=multi`, this can still be overridden by explicitly setting the latter configuration to either "single" or "false". Tests covering all of these cases are showin t5332. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/config/feature.txt | 3 +++ builtin/pack-objects.c | 2 ++ repo-settings.c | 1 + repository.h | 1 + t/t5332-multi-pack-reuse.sh | 16 ++++++++++++++++ 5 files changed, 23 insertions(+)