Skip to content

Commit 481d69d

Browse files
gitstergitster
committed
Merge branch 'ps/reftable-fixes-and-optims'
More fixes and optimizations to the reftable backend. * ps/reftable-fixes-and-optims: reftable/merged: transfer ownership of records when iterating reftable/merged: really reuse buffers to compute record keys reftable/record: store "val2" hashes as static arrays reftable/record: store "val1" hashes as static arrays reftable/record: constify some parts of the interface reftable/writer: fix index corruption when writing multiple indices reftable/stack: do not auto-compact twice in `reftable_stack_add()` reftable/stack: do not overwrite errors when compacting
2 parents d4dbce1 + 19b9496 commit 481d69d

File tree

10 files changed

+117
-75
lines changed

10 files changed

+117
-75
lines changed

‎reftable/block_test.c

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,13 +49,11 @@ static void test_block_read_write(void)
4949

5050
for (i = 0; i < N; i++) {
5151
char name[100];
52-
uint8_t hash[GIT_SHA1_RAWSZ];
5352
snprintf(name, sizeof(name), "branch%02d", i);
54-
memset(hash, i, sizeof(hash));
5553

5654
rec.u.ref.refname = name;
5755
rec.u.ref.value_type = REFTABLE_REF_VAL1;
58-
rec.u.ref.value.val1 = hash;
56+
memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
5957

6058
names[i] = xstrdup(name);
6159
n = block_writer_add(&bw, &rec);

‎reftable/merged.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,12 +123,12 @@ static int merged_iter_next_entry(struct merged_iter *mi,
123123
reftable_record_release(&top.rec);
124124
}
125125

126-
reftable_record_copy_from(rec, &entry.rec, hash_size(mi->hash_id));
126+
reftable_record_release(rec);
127+
*rec = entry.rec;
127128

128129
done:
129-
reftable_record_release(&entry.rec);
130-
strbuf_release(&mi->entry_key);
131-
strbuf_release(&mi->key);
130+
if (err)
131+
reftable_record_release(&entry.rec);
132132
return err;
133133
}
134134

‎reftable/merged_test.c

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,11 @@ static void readers_destroy(struct reftable_reader **readers, size_t n)
122122

123123
static void test_merged_between(void)
124124
{
125-
uint8_t hash1[GIT_SHA1_RAWSZ] = { 1, 2, 3, 0 };
126-
127125
struct reftable_ref_record r1[] = { {
128126
.refname = "b",
129127
.update_index = 1,
130128
.value_type = REFTABLE_REF_VAL1,
131-
.value.val1 = hash1,
129+
.value.val1 = { 1, 2, 3, 0 },
132130
} };
133131
struct reftable_ref_record r2[] = { {
134132
.refname = "a",
@@ -164,26 +162,24 @@ static void test_merged_between(void)
164162

165163
static void test_merged(void)
166164
{
167-
uint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };
168-
uint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };
169165
struct reftable_ref_record r1[] = {
170166
{
171167
.refname = "a",
172168
.update_index = 1,
173169
.value_type = REFTABLE_REF_VAL1,
174-
.value.val1 = hash1,
170+
.value.val1 = { 1 },
175171
},
176172
{
177173
.refname = "b",
178174
.update_index = 1,
179175
.value_type = REFTABLE_REF_VAL1,
180-
.value.val1 = hash1,
176+
.value.val1 = { 1 },
181177
},
182178
{
183179
.refname = "c",
184180
.update_index = 1,
185181
.value_type = REFTABLE_REF_VAL1,
186-
.value.val1 = hash1,
182+
.value.val1 = { 1 },
187183
}
188184
};
189185
struct reftable_ref_record r2[] = { {
@@ -196,13 +192,13 @@ static void test_merged(void)
196192
.refname = "c",
197193
.update_index = 3,
198194
.value_type = REFTABLE_REF_VAL1,
199-
.value.val1 = hash2,
195+
.value.val1 = { 2 },
200196
},
201197
{
202198
.refname = "d",
203199
.update_index = 3,
204200
.value_type = REFTABLE_REF_VAL1,
205-
.value.val1 = hash1,
201+
.value.val1 = { 1 },
206202
},
207203
};
208204

‎reftable/readwrite_test.c

Lines changed: 86 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -59,18 +59,15 @@ static void write_table(char ***names, struct strbuf *buf, int N,
5959
*names = reftable_calloc(sizeof(char *) * (N + 1));
6060
reftable_writer_set_limits(w, update_index, update_index);
6161
for (i = 0; i < N; i++) {
62-
uint8_t hash[GIT_SHA256_RAWSZ] = { 0 };
6362
char name[100];
6463
int n;
6564

66-
set_test_hash(hash, i);
67-
6865
snprintf(name, sizeof(name), "refs/heads/branch%02d", i);
6966

7067
ref.refname = name;
7168
ref.update_index = update_index;
7269
ref.value_type = REFTABLE_REF_VAL1;
73-
ref.value.val1 = hash;
70+
set_test_hash(ref.value.val1, i);
7471
(*names)[i] = xstrdup(name);
7572

7673
n = reftable_writer_add_ref(w, &ref);
@@ -549,8 +546,6 @@ static void test_table_refs_for(int indexed)
549546
uint8_t hash[GIT_SHA1_RAWSZ];
550547
char fill[51] = { 0 };
551548
char name[100];
552-
uint8_t hash1[GIT_SHA1_RAWSZ];
553-
uint8_t hash2[GIT_SHA1_RAWSZ];
554549
struct reftable_ref_record ref = { NULL };
555550

556551
memset(hash, i, sizeof(hash));
@@ -560,20 +555,18 @@ static void test_table_refs_for(int indexed)
560555
name[40] = 0;
561556
ref.refname = name;
562557

563-
set_test_hash(hash1, i / 4);
564-
set_test_hash(hash2, 3 + i / 4);
565558
ref.value_type = REFTABLE_REF_VAL2;
566-
ref.value.val2.value = hash1;
567-
ref.value.val2.target_value = hash2;
559+
set_test_hash(ref.value.val2.value, i / 4);
560+
set_test_hash(ref.value.val2.target_value, 3 + i / 4);
568561

569562
/* 80 bytes / entry, so 3 entries per block. Yields 17
570563
*/
571564
/* blocks. */
572565
n = reftable_writer_add_ref(w, &ref);
573566
EXPECT(n == 0);
574567

575-
if (!memcmp(hash1, want_hash, GIT_SHA1_RAWSZ) ||
576-
!memcmp(hash2, want_hash, GIT_SHA1_RAWSZ)) {
568+
if (!memcmp(ref.value.val2.value, want_hash, GIT_SHA1_RAWSZ) ||
569+
!memcmp(ref.value.val2.target_value, want_hash, GIT_SHA1_RAWSZ)) {
577570
want_names[want_names_len++] = xstrdup(name);
578571
}
579572
}
@@ -674,11 +667,10 @@ static void test_write_object_id_min_length(void)
674667
struct strbuf buf = STRBUF_INIT;
675668
struct reftable_writer *w =
676669
reftable_new_writer(&strbuf_add_void, &buf, &opts);
677-
uint8_t hash[GIT_SHA1_RAWSZ] = {42};
678670
struct reftable_ref_record ref = {
679671
.update_index = 1,
680672
.value_type = REFTABLE_REF_VAL1,
681-
.value.val1 = hash,
673+
.value.val1 = {42},
682674
};
683675
int err;
684676
int i;
@@ -710,11 +702,10 @@ static void test_write_object_id_length(void)
710702
struct strbuf buf = STRBUF_INIT;
711703
struct reftable_writer *w =
712704
reftable_new_writer(&strbuf_add_void, &buf, &opts);
713-
uint8_t hash[GIT_SHA1_RAWSZ] = {42};
714705
struct reftable_ref_record ref = {
715706
.update_index = 1,
716707
.value_type = REFTABLE_REF_VAL1,
717-
.value.val1 = hash,
708+
.value.val1 = {42},
718709
};
719710
int err;
720711
int i;
@@ -797,6 +788,84 @@ static void test_write_key_order(void)
797788
strbuf_release(&buf);
798789
}
799790

791+
static void test_write_multiple_indices(void)
792+
{
793+
struct reftable_write_options opts = {
794+
.block_size = 100,
795+
};
796+
struct strbuf writer_buf = STRBUF_INIT, buf = STRBUF_INIT;
797+
struct reftable_block_source source = { 0 };
798+
struct reftable_iterator it = { 0 };
799+
const struct reftable_stats *stats;
800+
struct reftable_writer *writer;
801+
struct reftable_reader *reader;
802+
int err, i;
803+
804+
writer = reftable_new_writer(&strbuf_add_void, &writer_buf, &opts);
805+
reftable_writer_set_limits(writer, 1, 1);
806+
for (i = 0; i < 100; i++) {
807+
struct reftable_ref_record ref = {
808+
.update_index = 1,
809+
.value_type = REFTABLE_REF_VAL1,
810+
.value.val1 = {i},
811+
};
812+
813+
strbuf_reset(&buf);
814+
strbuf_addf(&buf, "refs/heads/%04d", i);
815+
ref.refname = buf.buf,
816+
817+
err = reftable_writer_add_ref(writer, &ref);
818+
EXPECT_ERR(err);
819+
}
820+
821+
for (i = 0; i < 100; i++) {
822+
unsigned char hash[GIT_SHA1_RAWSZ] = {i};
823+
struct reftable_log_record log = {
824+
.update_index = 1,
825+
.value_type = REFTABLE_LOG_UPDATE,
826+
.value.update = {
827+
.old_hash = hash,
828+
.new_hash = hash,
829+
},
830+
};
831+
832+
strbuf_reset(&buf);
833+
strbuf_addf(&buf, "refs/heads/%04d", i);
834+
log.refname = buf.buf,
835+
836+
err = reftable_writer_add_log(writer, &log);
837+
EXPECT_ERR(err);
838+
}
839+
840+
reftable_writer_close(writer);
841+
842+
/*
843+
* The written data should be sufficiently large to result in indices
844+
* for each of the block types.
845+
*/
846+
stats = reftable_writer_stats(writer);
847+
EXPECT(stats->ref_stats.index_offset > 0);
848+
EXPECT(stats->obj_stats.index_offset > 0);
849+
EXPECT(stats->log_stats.index_offset > 0);
850+
851+
block_source_from_strbuf(&source, &writer_buf);
852+
err = reftable_new_reader(&reader, &source, "filename");
853+
EXPECT_ERR(err);
854+
855+
/*
856+
* Seeking the log uses the log index now. In case there is any
857+
* confusion regarding indices we would notice here.
858+
*/
859+
err = reftable_reader_seek_log(reader, &it, "");
860+
EXPECT_ERR(err);
861+
862+
reftable_iterator_destroy(&it);
863+
reftable_writer_free(writer);
864+
reftable_reader_free(reader);
865+
strbuf_release(&writer_buf);
866+
strbuf_release(&buf);
867+
}
868+
800869
static void test_corrupt_table_empty(void)
801870
{
802871
struct strbuf buf = STRBUF_INIT;
@@ -846,5 +915,6 @@ int readwrite_test_main(int argc, const char *argv[])
846915
RUN_TEST(test_log_overflow);
847916
RUN_TEST(test_write_object_id_length);
848917
RUN_TEST(test_write_object_id_min_length);
918+
RUN_TEST(test_write_multiple_indices);
849919
return 0;
850920
}

‎reftable/record.c

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ int reftable_is_block_type(uint8_t typ)
7676
return 0;
7777
}
7878

79-
uint8_t *reftable_ref_record_val1(const struct reftable_ref_record *rec)
79+
const unsigned char *reftable_ref_record_val1(const struct reftable_ref_record *rec)
8080
{
8181
switch (rec->value_type) {
8282
case REFTABLE_REF_VAL1:
@@ -88,7 +88,7 @@ uint8_t *reftable_ref_record_val1(const struct reftable_ref_record *rec)
8888
}
8989
}
9090

91-
uint8_t *reftable_ref_record_val2(const struct reftable_ref_record *rec)
91+
const unsigned char *reftable_ref_record_val2(const struct reftable_ref_record *rec)
9292
{
9393
switch (rec->value_type) {
9494
case REFTABLE_REF_VAL2:
@@ -219,13 +219,10 @@ static void reftable_ref_record_copy_from(void *rec, const void *src_rec,
219219
case REFTABLE_REF_DELETION:
220220
break;
221221
case REFTABLE_REF_VAL1:
222-
ref->value.val1 = reftable_malloc(hash_size);
223222
memcpy(ref->value.val1, src->value.val1, hash_size);
224223
break;
225224
case REFTABLE_REF_VAL2:
226-
ref->value.val2.value = reftable_malloc(hash_size);
227225
memcpy(ref->value.val2.value, src->value.val2.value, hash_size);
228-
ref->value.val2.target_value = reftable_malloc(hash_size);
229226
memcpy(ref->value.val2.target_value,
230227
src->value.val2.target_value, hash_size);
231228
break;
@@ -242,7 +239,7 @@ static char hexdigit(int c)
242239
return 'a' + (c - 10);
243240
}
244241

245-
static void hex_format(char *dest, uint8_t *src, int hash_size)
242+
static void hex_format(char *dest, const unsigned char *src, int hash_size)
246243
{
247244
assert(hash_size > 0);
248245
if (src) {
@@ -299,11 +296,8 @@ void reftable_ref_record_release(struct reftable_ref_record *ref)
299296
reftable_free(ref->value.symref);
300297
break;
301298
case REFTABLE_REF_VAL2:
302-
reftable_free(ref->value.val2.target_value);
303-
reftable_free(ref->value.val2.value);
304299
break;
305300
case REFTABLE_REF_VAL1:
306-
reftable_free(ref->value.val1);
307301
break;
308302
case REFTABLE_REF_DELETION:
309303
break;
@@ -394,7 +388,6 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,
394388
return -1;
395389
}
396390

397-
r->value.val1 = reftable_malloc(hash_size);
398391
memcpy(r->value.val1, in.buf, hash_size);
399392
string_view_consume(&in, hash_size);
400393
break;
@@ -404,11 +397,9 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,
404397
return -1;
405398
}
406399

407-
r->value.val2.value = reftable_malloc(hash_size);
408400
memcpy(r->value.val2.value, in.buf, hash_size);
409401
string_view_consume(&in, hash_size);
410402

411-
r->value.val2.target_value = reftable_malloc(hash_size);
412403
memcpy(r->value.val2.target_value, in.buf, hash_size);
413404
string_view_consume(&in, hash_size);
414405
break;
@@ -1164,7 +1155,7 @@ int reftable_record_equal(struct reftable_record *a, struct reftable_record *b,
11641155
reftable_record_data(a), reftable_record_data(b), hash_size);
11651156
}
11661157

1167-
static int hash_equal(uint8_t *a, uint8_t *b, int hash_size)
1158+
static int hash_equal(const unsigned char *a, const unsigned char *b, int hash_size)
11681159
{
11691160
if (a && b)
11701161
return !memcmp(a, b, hash_size);

‎reftable/record_test.c

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,15 +119,10 @@ static void test_reftable_ref_record_roundtrip(void)
119119
case REFTABLE_REF_DELETION:
120120
break;
121121
case REFTABLE_REF_VAL1:
122-
in.u.ref.value.val1 = reftable_malloc(GIT_SHA1_RAWSZ);
123122
set_hash(in.u.ref.value.val1, 1);
124123
break;
125124
case REFTABLE_REF_VAL2:
126-
in.u.ref.value.val2.value =
127-
reftable_malloc(GIT_SHA1_RAWSZ);
128125
set_hash(in.u.ref.value.val2.value, 1);
129-
in.u.ref.value.val2.target_value =
130-
reftable_malloc(GIT_SHA1_RAWSZ);
131126
set_hash(in.u.ref.value.val2.target_value, 2);
132127
break;
133128
case REFTABLE_REF_SYMREF:

‎reftable/reftable-record.h

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ license that can be found in the LICENSE file or at
99
#ifndef REFTABLE_RECORD_H
1010
#define REFTABLE_RECORD_H
1111

12+
#include "hash-ll.h"
1213
#include <stdint.h>
1314

1415
/*
@@ -38,22 +39,22 @@ struct reftable_ref_record {
3839
#define REFTABLE_NR_REF_VALUETYPES 4
3940
} value_type;
4041
union {
41-
uint8_t *val1; /* malloced hash. */
42+
unsigned char val1[GIT_MAX_RAWSZ];
4243
struct {
43-
uint8_t *value; /* first value, malloced hash */
44-
uint8_t *target_value; /* second value, malloced hash */
44+
unsigned char value[GIT_MAX_RAWSZ]; /* first hash */
45+
unsigned char target_value[GIT_MAX_RAWSZ]; /* second hash */
4546
} val2;
4647
char *symref; /* referent, malloced 0-terminated string */
4748
} value;
4849
};
4950

5051
/* Returns the first hash, or NULL if `rec` is not of type
5152
* REFTABLE_REF_VAL1 or REFTABLE_REF_VAL2. */
52-
uint8_t *reftable_ref_record_val1(const struct reftable_ref_record *rec);
53+
const unsigned char *reftable_ref_record_val1(const struct reftable_ref_record *rec);
5354

5455
/* Returns the second hash, or NULL if `rec` is not of type
5556
* REFTABLE_REF_VAL2. */
56-
uint8_t *reftable_ref_record_val2(const struct reftable_ref_record *rec);
57+
const unsigned char *reftable_ref_record_val2(const struct reftable_ref_record *rec);
5758

5859
/* returns whether 'ref' represents a deletion */
5960
int reftable_ref_record_is_deletion(const struct reftable_ref_record *ref);

0 commit comments

Comments
 (0)