| Message ID | 9a33914c852a0487dbd90c83f53fa0e36414fda1.1711685809.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable/stack: use geometric table compaction | expand |
"Justin Tobler via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Justin Tobler <jltobler@gmail.com> > > For a reftable segment, the start of the range is inclusive and the end > is exclusive. In practice we increment the end when creating the > compaction segment only to decrement the segment end when using it. > > Simplify by making the segment end inclusive. The corresponding test, > `test_suggest_compaction_segment()`, is updated to show that the segment > end is now inclusive. > > Signed-off-by: Justin Tobler <jltobler@gmail.com> > --- > reftable/stack.c | 4 ++-- > reftable/stack_test.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) I'd defer it to Patrick (and Han-Wen, if he wants to comment on it), but isn't it a natural expectation shared among CS folks that it is the most usual way to express a range to use inclusive lower-end and exclusive upper-end? After all, that is how an array works, i.e. msg[n] is NULL and beyond the end where n == strlen(msg). So, I dunno. > diff --git a/reftable/stack.c b/reftable/stack.c > index e7b9a1de5a4..0973c47dd92 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -1237,7 +1237,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n) > */ > for (i = n - 1; i > 0; i--) { > if (sizes[i - 1] < sizes[i] * 2) { > - seg.end = i + 1; > + seg.end = i; > bytes = sizes[i]; > break; > } > @@ -1291,7 +1291,7 @@ int reftable_stack_auto_compact(struct reftable_stack *st) > suggest_compaction_segment(sizes, st->merged->stack_len); > reftable_free(sizes); > if (segment_size(&seg) > 0) > - return stack_compact_range_stats(st, seg.start, seg.end - 1, > + return stack_compact_range_stats(st, seg.start, seg.end, > NULL); > > return 0; > diff --git a/reftable/stack_test.c b/reftable/stack_test.c > index 21541742fe5..4d7305623a0 100644 > --- a/reftable/stack_test.c > +++ b/reftable/stack_test.c > @@ -723,7 +723,7 @@ static void test_suggest_compaction_segment(void) > struct segment min = > suggest_compaction_segment(sizes, ARRAY_SIZE(sizes)); > EXPECT(min.start == 1); > - EXPECT(min.end == 10); > + EXPECT(min.end == 9); > } > > static void test_suggest_compaction_segment_nothing(void)
On Fri, Mar 29, 2024 at 11:36:33AM -0700, Junio C Hamano wrote: > "Justin Tobler via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Justin Tobler <jltobler@gmail.com> > > > > For a reftable segment, the start of the range is inclusive and the end > > is exclusive. In practice we increment the end when creating the > > compaction segment only to decrement the segment end when using it. > > > > Simplify by making the segment end inclusive. The corresponding test, > > `test_suggest_compaction_segment()`, is updated to show that the segment > > end is now inclusive. > > > > Signed-off-by: Justin Tobler <jltobler@gmail.com> > > --- > > reftable/stack.c | 4 ++-- > > reftable/stack_test.c | 2 +- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > I'd defer it to Patrick (and Han-Wen, if he wants to comment on it), > but isn't it a natural expectation shared among CS folks that it is > the most usual way to express a range to use inclusive lower-end and > exclusive upper-end? > > After all, that is how an array works, i.e. msg[n] is NULL and > beyond the end where n == strlen(msg). > > So, I dunno. I don't really have a strong opinion here, to be honest. I think the previous way to handle this was fine, the new way is okay, too. Which may indicate that we can just drop this patch to avoid needless churn unless somebody feels strongly about this. Patrick > > diff --git a/reftable/stack.c b/reftable/stack.c > > index e7b9a1de5a4..0973c47dd92 100644 > > --- a/reftable/stack.c > > +++ b/reftable/stack.c > > @@ -1237,7 +1237,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n) > > */ > > for (i = n - 1; i > 0; i--) { > > if (sizes[i - 1] < sizes[i] * 2) { > > - seg.end = i + 1; > > + seg.end = i; > > bytes = sizes[i]; > > break; > > } > > > > > @@ -1291,7 +1291,7 @@ int reftable_stack_auto_compact(struct reftable_stack *st) > > suggest_compaction_segment(sizes, st->merged->stack_len); > > reftable_free(sizes); > > if (segment_size(&seg) > 0) > > - return stack_compact_range_stats(st, seg.start, seg.end - 1, > > + return stack_compact_range_stats(st, seg.start, seg.end, > > NULL); > > > > return 0; > > diff --git a/reftable/stack_test.c b/reftable/stack_test.c > > index 21541742fe5..4d7305623a0 100644 > > --- a/reftable/stack_test.c > > +++ b/reftable/stack_test.c > > @@ -723,7 +723,7 @@ static void test_suggest_compaction_segment(void) > > struct segment min = > > suggest_compaction_segment(sizes, ARRAY_SIZE(sizes)); > > EXPECT(min.start == 1); > > - EXPECT(min.end == 10); > > + EXPECT(min.end == 9); > > } > > > > static void test_suggest_compaction_segment_nothing(void)
diff --git a/reftable/stack.c b/reftable/stack.c index e7b9a1de5a4..0973c47dd92 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -1237,7 +1237,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n) */ for (i = n - 1; i > 0; i--) { if (sizes[i - 1] < sizes[i] * 2) { - seg.end = i + 1; + seg.end = i; bytes = sizes[i]; break; } @@ -1291,7 +1291,7 @@ int reftable_stack_auto_compact(struct reftable_stack *st) suggest_compaction_segment(sizes, st->merged->stack_len); reftable_free(sizes); if (segment_size(&seg) > 0) - return stack_compact_range_stats(st, seg.start, seg.end - 1, + return stack_compact_range_stats(st, seg.start, seg.end, NULL); return 0; diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 21541742fe5..4d7305623a0 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -723,7 +723,7 @@ static void test_suggest_compaction_segment(void) struct segment min = suggest_compaction_segment(sizes, ARRAY_SIZE(sizes)); EXPECT(min.start == 1); - EXPECT(min.end == 10); + EXPECT(min.end == 9); } static void test_suggest_compaction_segment_nothing(void)