From: Taylor Blau <me@ttaylorr.com>
To: Abhradeep Chakraborty via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Taylor Blau" <me@ttaylorr.com>,
"Kaartic Sivaram" <kaartic.sivaraam@gmail.com>,
"Derrick Stolee" <derrickstolee@github.com>,
"Philip Oakley" <philipoakley@iee.email>,
"Martin Ågren" <martin.agren@gmail.com>,
"Abhradeep Chakraborty" <chakrabortyabhradeep79@gmail.com>
Subject: Re: [PATCH v5 4/6] pack-bitmap: prepare to read lookup table extension
Date: Mon, 25 Jul 2022 21:13:50 -0400 [thread overview]
Message-ID: <Yt8/zl/uOLRBRS4h@nand.local> (raw)
In-Reply-To: <6918f0860adbea1156fb7a9f1a5aedd211872481.1658342304.git.gitgitgadget@gmail.com>
On Wed, Jul 20, 2022 at 06:38:22PM +0000, Abhradeep Chakraborty via GitGitGadget wrote:
> @@ -557,13 +575,238 @@ struct include_data {
> struct bitmap *seen;
> };
>
> +struct bitmap_lookup_table_triplet {
> + uint32_t commit_pos;
> + uint64_t offset;
> + uint32_t xor_row;
> +};
> +
> +struct bitmap_lookup_table_xor_item {
> + struct object_id oid;
> + uint64_t offset;
> +};
> +
> +/*
> + * Given a `triplet` struct pointer and pointer `p`, this
> + * function reads the triplet beginning at `p` into the struct.
> + * Note that this function assumes that there is enough memory
> + * left for filling the `triplet` struct from `p`.
> + */
> +static int lookup_table_get_triplet_by_pointer(struct bitmap_lookup_table_triplet *triplet,
> + const unsigned char *p)
> +{
> + if (!triplet)
> + return -1;
> +
> + triplet->commit_pos = get_be32(p);
> + p += sizeof(uint32_t);
> + triplet->offset = get_be64(p);
> + p += sizeof(uint64_t);
> + triplet->xor_row = get_be32(p);
> + return 0;
Just noticing this now, but I wonder if we could avoid incrementing `p`
here and instead write something like:
triplet->commit_pos = get_be32(p);
triplet->offset = get_be64(p + sizeof(uint32_t));
triplet->xor_row = get_be64(p + sizeof(uint64_t) + sizeof(uint32_t));
I don't have a strong feeling about it, though, it just seems to read a
little more directly to me and avoid modifying a variable that is only
going to live as long as the function executes (p).
> +/*
> + * This function gets the raw triplet from `row`'th row in the
> + * lookup table and fills that data to the `triplet`.
> + */
> +static int lookup_table_get_triplet(struct bitmap_index *bitmap_git,
> + uint32_t pos,
> + struct bitmap_lookup_table_triplet *triplet)
> +{
> + unsigned char *p = NULL;
> + if (pos >= bitmap_git->entry_count)
> + return error(_("corrupt bitmap lookup table: triplet position out of index"));
> +
> + p = bitmap_git->table_lookup + st_mult(pos, BITMAP_LOOKUP_TABLE_TRIPLET_WIDTH);
> +
> + return lookup_table_get_triplet_by_pointer(triplet, p);
> +}
Very nice. This cleans things up nicely by being able to call
lookup_table_get_triplet_by_pointer().
Since these are static functions, it doesn't really matter whether or
not they are prefixed with 'bitmap_', since they won't be visible
outside of pack-bitmap.c's compilation unit. But it may be nice to
prefix them with 'bitmap_' just to make it extra clear that these are
internal functions meant to be used within the bitmap machinery only.
> +static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_git,
> + struct commit *commit)
> +{
> + uint32_t commit_pos, xor_row;
> + uint64_t offset;
> + int flags, found;
> + struct bitmap_lookup_table_triplet triplet;
> + struct object_id *oid = &commit->object.oid;
> + struct ewah_bitmap *bitmap;
> + struct stored_bitmap *xor_bitmap = NULL;
> + const int bitmap_header_size = 6;
> + static struct bitmap_lookup_table_xor_item *xor_items = NULL;
> + static size_t xor_items_nr = 0, xor_items_alloc = 0;
I had to double check, but xor_items_alloc inherits the same storage
class at xor_items_nr, so they are both static size_t's.
> + static int is_corrupt = 0;
> +
> + if (is_corrupt)
> + return NULL;
What is the purpose of this conditional? We don't modify `is_corrupt`
before reading it here, so this should be dead code, unless I'm missing
something.
> + found = bsearch_pos(bitmap_git, oid, &commit_pos);
> +
> + if (!found)
> + return NULL;
FWIW, we could eliminate this variable from a particularly long list of
stack variables above by just writing:
if (!bsearch_pos(bitmap_git, oid, &commit_pos))
return NULL;
and I think that would be OK.
> + if (bsearch_triplet_by_pos(commit_pos, bitmap_git, &triplet) < 0)
> + return NULL;
> +
> + xor_items_nr = 0;
This initialization *is* necessary, since the xor_items_nr and
xor_items_alloc variable are statically allocated.
> + offset = triplet.offset;
> + xor_row = triplet.xor_row;
> +
> + if (xor_row != 0xffffffff) {
Is this outer conditional needed? I don't think it is. If xor_row is
0xffffffff, then the while loop below won't be entered, and
xor_items_nr will be zero, meaning that the second while loop will also
be skipped.
So I think we can just as easily get rid of this outermost if-statement
and de-dent the main part of this function's body.
> + int xor_flags;
> + khiter_t hash_pos;
> + struct bitmap_lookup_table_xor_item *xor_item;
> +
> + while (xor_row != 0xffffffff) {
> + ALLOC_GROW(xor_items, xor_items_nr + 1, xor_items_alloc);
> +
> + if (xor_items_nr + 1 >= bitmap_git->entry_count) {
> + error(_("corrupt bitmap lookup table: xor chain exceed entry count"));
> + goto corrupt;
> + }
> +
> + if (lookup_table_get_triplet(bitmap_git, xor_row, &triplet) < 0)
> + goto corrupt;
> +
> + xor_item = &xor_items[xor_items_nr];
> + xor_item->offset = triplet.offset;
> +
> + if (nth_bitmap_object_oid(bitmap_git, &xor_item->oid, triplet.commit_pos) < 0) {
> + error(_("corrupt bitmap lookup table: commit index %u out of range"),
> + triplet.commit_pos);
> + goto corrupt;
> + }
> +
> + hash_pos = kh_get_oid_map(bitmap_git->bitmaps, xor_item->oid);
> +
> + /*
> + * If desired bitmap is already stored, we don't need
> + * to iterate further. Because we know that bitmaps
> + * that are needed to be parsed to parse this bitmap
> + * has already been stored. So, assign this stored bitmap
> + * to the xor_bitmap.
> + */
> + if (hash_pos < kh_end(bitmap_git->bitmaps) &&
> + (xor_bitmap = kh_value(bitmap_git->bitmaps, hash_pos)))
> + break;
> + xor_items_nr++;
> + xor_row = triplet.xor_row;
> + }
> +
> + while (xor_items_nr) {
> + xor_item = &xor_items[xor_items_nr - 1];
> + bitmap_git->map_pos = xor_item->offset;
> + if (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size) {
> + error(_("corrupt ewah bitmap: truncated header for bitmap of commit \"%s\""),
> + oid_to_hex(&xor_item->oid));
> + goto corrupt;
> + }
> +
> + bitmap_git->map_pos = bitmap_git->map_pos + sizeof(uint32_t) + sizeof(uint8_t);
Could we write:
bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t)
or similar? Also, a clarifying comment would help explain why we are
advancing, what we're skipping past, and why it's OK.
> + xor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);
> + bitmap = read_bitmap_1(bitmap_git);
> +
> + if (!bitmap)
> + goto corrupt;
> +
> + xor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, xor_bitmap, xor_flags);
> + xor_items_nr--;
> + }
> + }
> +
> + bitmap_git->map_pos = offset;
> + if (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size) {
> + error(_("corrupt ewah bitmap: truncated header for bitmap of commit \"%s\""),
> + oid_to_hex(oid));
> + goto corrupt;
> + }
> +
> + bitmap_git->map_pos = bitmap_git->map_pos + sizeof(uint32_t) + sizeof(uint8_t);
> + flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);
> + bitmap = read_bitmap_1(bitmap_git);
> +
> + if (!bitmap)
> + goto corrupt;
> +
> + return store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags);
> +
> +corrupt:
> + free(xor_items);
> + is_corrupt = 1;
> + return NULL;
> +}
Thanks,
Taylor
next prev parent reply other threads:[~2022-07-26 1:13 UTC|newest]
Thread overview: 162+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-20 12:33 [PATCH 0/6] [GSoC] bitmap: integrate a lookup table extension to the bitmap format Abhradeep Chakraborty via GitGitGadget
2022-06-20 12:33 ` [PATCH 1/6] Documentation/technical: describe bitmap lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-06-20 16:56 ` Derrick Stolee
2022-06-20 17:09 ` Taylor Blau
2022-06-21 8:31 ` Abhradeep Chakraborty
2022-06-22 16:26 ` Taylor Blau
2022-06-21 8:23 ` Abhradeep Chakraborty
2022-06-20 17:21 ` Taylor Blau
2022-06-21 9:22 ` Abhradeep Chakraborty
2022-06-22 16:29 ` Taylor Blau
2022-06-22 16:45 ` Abhradeep Chakraborty
2022-06-20 20:21 ` Derrick Stolee
2022-06-21 10:08 ` Abhradeep Chakraborty
2022-06-22 16:30 ` Taylor Blau
2022-06-20 12:33 ` [PATCH 2/6] pack-bitmap: prepare to read " Abhradeep Chakraborty via GitGitGadget
2022-06-20 20:49 ` Derrick Stolee
2022-06-21 10:28 ` Abhradeep Chakraborty
2022-06-20 22:06 ` Taylor Blau
2022-06-21 11:52 ` Abhradeep Chakraborty
2022-06-22 16:49 ` Taylor Blau
2022-06-22 17:18 ` Abhradeep Chakraborty
2022-06-22 21:34 ` Taylor Blau
2022-06-20 12:33 ` [PATCH 3/6] pack-bitmap-write.c: write " Abhradeep Chakraborty via GitGitGadget
2022-06-20 22:16 ` Taylor Blau
2022-06-21 12:50 ` Abhradeep Chakraborty
2022-06-22 16:51 ` Taylor Blau
2022-06-20 12:33 ` [PATCH 4/6] builtin/pack-objects.c: learn pack.writeBitmapLookupTable Taylor Blau via GitGitGadget
2022-06-20 22:18 ` Taylor Blau
2022-06-20 12:33 ` [PATCH 5/6] bitmap-commit-table: add tests for the bitmap lookup table Abhradeep Chakraborty via GitGitGadget
2022-06-22 16:54 ` Taylor Blau
2022-06-20 12:33 ` [PATCH 6/6] bitmap-lookup-table: add performance tests Abhradeep Chakraborty via GitGitGadget
2022-06-22 17:14 ` Taylor Blau
2022-06-26 13:10 ` [PATCH v2 0/6] [GSoC] bitmap: integrate a lookup table extension to the bitmap format Abhradeep Chakraborty via GitGitGadget
2022-06-26 13:10 ` [PATCH v2 1/6] Documentation/technical: describe bitmap lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-06-27 14:18 ` Derrick Stolee
2022-06-27 15:48 ` Taylor Blau
2022-06-27 16:51 ` Abhradeep Chakraborty
2022-06-26 13:10 ` [PATCH v2 2/6] pack-bitmap-write.c: write " Abhradeep Chakraborty via GitGitGadget
2022-06-27 14:35 ` Derrick Stolee
2022-06-27 16:12 ` Taylor Blau
2022-06-27 17:10 ` Abhradeep Chakraborty
2022-06-27 16:05 ` Taylor Blau
2022-06-27 18:29 ` Abhradeep Chakraborty
2022-06-26 13:10 ` [PATCH v2 3/6] pack-bitmap-write: learn pack.writeBitmapLookupTable and add tests Abhradeep Chakraborty via GitGitGadget
2022-06-27 14:43 ` Derrick Stolee
2022-06-27 17:42 ` Abhradeep Chakraborty
2022-06-27 17:49 ` Taylor Blau
2022-06-27 17:47 ` Taylor Blau
2022-06-27 18:39 ` Abhradeep Chakraborty
2022-06-29 20:11 ` Taylor Blau
2022-06-26 13:10 ` [PATCH v2 4/6] pack-bitmap: prepare to read lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-06-27 15:12 ` Derrick Stolee
2022-06-27 18:06 ` [PATCH v2 4/6] pack-bitmap: prepare to read lookup table Abhradeep Chakraborty
2022-06-27 18:32 ` Derrick Stolee
2022-06-27 21:49 ` [PATCH v2 4/6] pack-bitmap: prepare to read lookup table extension Taylor Blau
2022-06-28 8:59 ` [PATCH v2 4/6] pack-bitmap: prepare to read lookup table Abhradeep Chakraborty
2022-06-29 20:22 ` Taylor Blau
2022-06-30 6:58 ` [PATCH v2 4/6] pack-bitmap: prepare to read lookup table extension Abhradeep Chakraborty
2022-06-27 21:38 ` Taylor Blau
2022-06-28 19:25 ` Abhradeep Chakraborty
2022-06-29 20:37 ` Taylor Blau
2022-06-29 20:41 ` Taylor Blau
2022-06-30 8:35 ` Abhradeep Chakraborty
2022-06-26 13:10 ` [PATCH v2 5/6] bitmap-lookup-table: add performance tests for lookup table Abhradeep Chakraborty via GitGitGadget
2022-06-27 21:53 ` Taylor Blau
2022-06-28 7:58 ` Abhradeep Chakraborty
2022-06-29 20:40 ` Taylor Blau
2022-06-26 13:10 ` [PATCH v2 6/6] p5310-pack-bitmaps.sh: enable pack.writeReverseIndex for testing Abhradeep Chakraborty via GitGitGadget
2022-06-27 21:50 ` Taylor Blau
2022-06-28 8:01 ` Abhradeep Chakraborty
2022-07-04 8:46 ` [PATCH v3 0/6] [GSoC] bitmap: integrate a lookup table extension to the bitmap format Abhradeep Chakraborty via GitGitGadget
2022-07-04 8:46 ` [PATCH v3 1/6] Documentation/technical: describe bitmap lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-07-08 16:38 ` Philip Oakley
2022-07-09 7:53 ` Abhradeep Chakraborty
2022-07-10 15:01 ` Philip Oakley
2022-07-14 23:15 ` Taylor Blau
2022-07-15 10:36 ` Philip Oakley
2022-07-15 18:48 ` Abhradeep Chakraborty
2022-07-04 8:46 ` [PATCH v3 2/6] pack-bitmap-write.c: write " Abhradeep Chakraborty via GitGitGadget
2022-07-14 23:26 ` Taylor Blau
2022-07-15 2:22 ` Taylor Blau
2022-07-15 15:58 ` Abhradeep Chakraborty
2022-07-15 22:15 ` Taylor Blau
2022-07-16 11:50 ` Abhradeep Chakraborty
2022-07-26 0:34 ` Taylor Blau
2022-07-18 8:59 ` Martin Ågren
2022-07-04 8:46 ` [PATCH v3 3/6] pack-bitmap-write: learn pack.writeBitmapLookupTable and add tests Abhradeep Chakraborty via GitGitGadget
2022-07-04 8:46 ` [PATCH v3 4/6] pack-bitmap: prepare to read lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-07-15 2:46 ` Taylor Blau
2022-07-15 16:38 ` Abhradeep Chakraborty
2022-07-15 22:20 ` Taylor Blau
2022-07-18 9:06 ` Martin Ågren
2022-07-18 19:25 ` Abhradeep Chakraborty
2022-07-18 23:26 ` Martin Ågren
2022-07-26 0:45 ` Taylor Blau
2022-07-04 8:46 ` [PATCH v3 5/6] bitmap-lookup-table: add performance tests for lookup table Abhradeep Chakraborty via GitGitGadget
2022-07-15 2:53 ` Taylor Blau
2022-07-15 18:23 ` Abhradeep Chakraborty
2022-07-04 8:46 ` [PATCH v3 6/6] p5310-pack-bitmaps.sh: remove pack.writeReverseIndex Abhradeep Chakraborty via GitGitGadget
2022-07-04 16:35 ` [PATCH v3 0/6] [GSoC] bitmap: integrate a lookup table extension to the bitmap format Abhradeep Chakraborty
2022-07-06 19:21 ` Junio C Hamano
2022-07-07 8:48 ` Abhradeep Chakraborty
2022-07-07 18:09 ` Kaartic Sivaraam
2022-07-07 18:42 ` Abhradeep Chakraborty
2022-07-20 14:05 ` [PATCH v4 " Abhradeep Chakraborty via GitGitGadget
2022-07-20 14:05 ` [PATCH v4 1/6] Documentation/technical: describe bitmap lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-07-20 14:05 ` [PATCH v4 2/6] pack-bitmap-write.c: write " Abhradeep Chakraborty via GitGitGadget
2022-07-20 14:05 ` [PATCH v4 3/6] pack-bitmap-write: learn pack.writeBitmapLookupTable and add tests Abhradeep Chakraborty via GitGitGadget
2022-07-20 14:05 ` [PATCH v4 4/6] pack-bitmap: prepare to read lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-07-20 14:05 ` [PATCH v4 5/6] p5310-pack-bitmaps.sh: enable `pack.writeReverseIndex` Abhradeep Chakraborty via GitGitGadget
2022-07-20 14:05 ` [PATCH v4 6/6] bitmap-lookup-table: add performance tests for lookup table Abhradeep Chakraborty via GitGitGadget
2022-07-20 18:38 ` [PATCH v5 0/6] [GSoC] bitmap: integrate a lookup table extension to the bitmap format Abhradeep Chakraborty via GitGitGadget
2022-07-20 18:38 ` [PATCH v5 1/6] Documentation/technical: describe bitmap lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-07-20 18:38 ` [PATCH v5 2/6] pack-bitmap-write.c: write " Abhradeep Chakraborty via GitGitGadget
2022-07-26 0:52 ` Taylor Blau
2022-07-26 18:22 ` Abhradeep Chakraborty
2022-07-20 18:38 ` [PATCH v5 3/6] pack-bitmap-write: learn pack.writeBitmapLookupTable and add tests Abhradeep Chakraborty via GitGitGadget
2022-07-28 19:22 ` Johannes Schindelin
2022-08-02 12:40 ` Abhradeep Chakraborty
2022-08-02 15:35 ` Johannes Schindelin
2022-08-02 17:44 ` Abhradeep Chakraborty
2022-08-08 13:06 ` Johannes Schindelin
2022-08-08 13:58 ` Abhradeep Chakraborty
2022-08-09 9:03 ` Johannes Schindelin
2022-08-09 12:03 ` Abhradeep Chakraborty
2022-08-09 12:07 ` Abhradeep Chakraborty
2022-08-10 9:09 ` Johannes Schindelin
2022-08-10 9:20 ` Johannes Schindelin
2022-08-10 10:04 ` Abhradeep Chakraborty
2022-08-10 17:51 ` Derrick Stolee
2022-08-12 18:51 ` Abhradeep Chakraborty
2022-08-12 19:22 ` Derrick Stolee
2022-08-13 10:59 ` Abhradeep Chakraborty
2022-08-16 21:57 ` Taylor Blau
2022-08-17 10:02 ` Abhradeep Chakraborty
2022-08-17 20:38 ` Taylor Blau
2022-08-19 21:49 ` Taylor Blau
2022-08-13 11:05 ` Abhradeep Chakraborty
2022-08-16 18:47 ` Taylor Blau
2022-07-20 18:38 ` [PATCH v5 4/6] pack-bitmap: prepare to read lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-07-26 1:13 ` Taylor Blau [this message]
2022-07-26 18:56 ` Abhradeep Chakraborty
2022-07-26 19:36 ` Eric Sunshine
2022-07-20 18:38 ` [PATCH v5 5/6] p5310-pack-bitmaps.sh: enable `pack.writeReverseIndex` Abhradeep Chakraborty via GitGitGadget
2022-07-26 1:18 ` Taylor Blau
2022-07-26 7:15 ` Ævar Arnfjörð Bjarmason
2022-07-26 13:32 ` Derrick Stolee
2022-07-26 13:54 ` Ævar Arnfjörð Bjarmason
2022-07-26 18:17 ` Abhradeep Chakraborty
2022-07-20 18:38 ` [PATCH v5 6/6] bitmap-lookup-table: add performance tests for lookup table Abhradeep Chakraborty via GitGitGadget
2022-08-14 16:55 ` [PATCH v6 0/6] [GSoC] bitmap: integrate a lookup table extension to the bitmap format Abhradeep Chakraborty via GitGitGadget
2022-08-14 16:55 ` [PATCH v6 1/6] Documentation/technical: describe bitmap lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-08-14 16:55 ` [PATCH v6 2/6] bitmap: move `get commit positions` code to `bitmap_writer_finish` Abhradeep Chakraborty via GitGitGadget
2022-08-14 16:55 ` [PATCH v6 3/6] pack-bitmap-write.c: write lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-08-14 16:55 ` [PATCH v6 4/6] pack-bitmap-write: learn pack.writeBitmapLookupTable and add tests Abhradeep Chakraborty via GitGitGadget
2022-08-14 16:55 ` [PATCH v6 5/6] pack-bitmap: prepare to read lookup table extension Abhradeep Chakraborty via GitGitGadget
2022-08-14 16:55 ` [PATCH v6 6/6] bitmap-lookup-table: add performance tests for lookup table Abhradeep Chakraborty via GitGitGadget
2022-08-19 21:21 ` [PATCH v6 0/6] [GSoC] bitmap: integrate a lookup table extension to the bitmap format Junio C Hamano
2022-08-22 14:42 ` Johannes Schindelin
2022-08-22 14:48 ` Taylor Blau
2022-08-25 22:16 ` Taylor Blau
2022-08-26 16:02 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yt8/zl/uOLRBRS4h@nand.local \
--to=me@ttaylorr.com \
--cc=chakrabortyabhradeep79@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=kaartic.sivaraam@gmail.com \
--cc=martin.agren@gmail.com \
--cc=philipoakley@iee.email \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).