Re: [PATCH v2 1/6] Documentation/technical: describe bitmap lookup table extension

[Derrick Stolee <derrickstolee@github.com>]

On 6/26/2022 9:10 AM, Abhradeep Chakraborty via GitGitGadget wrote:
> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
> 
> When reading bitmap file, git loads each and every bitmap one by one
> even if all the bitmaps are not required. A "bitmap lookup table"
> extension to the bitmap format can reduce the overhead of loading
> bitmaps which stores a list of bitmapped commit id pos (in the midx
> or pack, along with their offset and xor offset. This way git can
> load only the neccesary bitmaps without loading the previous bitmaps.

s/neccesary/necessary/

> +			** {empty}
> +			BITMAP_OPT_LOOKUP_TABLE (0x10): :::
> +			If present, the end of the bitmap file contains a table
> +			containing a list of `N` <commit pos, offset, xor offset>

(Note that "commit pos" and "xor offset" here don't have underscores, but
your discussion below does use "xor_offset" with underscores.)

> +			triplets. The format and meaning of the table is described
> +			below.
> ++
> +NOTE: This xor_offset is different from the bitmap's xor_offset.
> +Bitmap's xor_offset is relative i.e. it tells how many bitmaps we have
> +to go back from the current bitmap. Lookup table's xor_offset tells the
> +position of the triplet in the list whose bitmap the current commit's
> +bitmap have to xor with.

I found this difficult to parse. Here is an attempt at a rewording. Please
let me know if I misunderstood something when reading your version:

  NOTE: The xor_offset stored in the BITMAP_OPT_LOOKUP_TABLE is different
  from the xor_offset used in the bitmap data table. The xor_offset in this
  table indicates the row number within this table of the commit whose
  bitmap is used for the XOR computation with the current commit's stored
  bitmap to create the proper logical reachability bitmap.

This does make me think that "xor_offset" should really be "xor_row" or
something like that.

>  		4-byte entry count (network byte order)
>  
>  			The total count of entries (bitmapped commits) in this bitmap index.
> @@ -205,3 +218,31 @@ Note that this hashing scheme is tied to the BITMAP_OPT_HASH_CACHE flag.
>  If implementations want to choose a different hashing scheme, they are
>  free to do so, but MUST allocate a new header flag (because comparing
>  hashes made under two different schemes would be pointless).
> +
> +Commit lookup table
> +-------------------
> +
> +If the BITMAP_OPT_LOOKUP_TABLE flag is set, the last `N * (4 + 8 + 4)`
> +(preceding the name-hash cache and trailing hash) of the `.bitmap` file
> +contains a lookup table specifying the information needed to get the
> +desired bitmap from the entries without parsing previous unnecessary
> +bitmaps.
> +
> +For a `.bitmap` containing `nr_entries` reachability bitmaps, the table
> +contains a list of `nr_entries` <commit pos, offset, xor offset> triplets.
> +The content of i'th triplet is -
> +
> +	* {empty}
> +	commit pos (4 byte integer, network byte order): ::
> +	It stores the object position of the commit (in the midx or pack index)
> +	to which the i'th bitmap in the bitmap entries belongs.

Ok, we are saving some space here, but relying on looking into the pack-index
or multi-pack-index to get the actual commit OID.

Since this is sorted by the order that stores the bitmaps, binary search will
no longer work on this list (unless we enforce that on the rest of the bitmap
file). I am going to expect that you parse this table into a hashmap in order
to allow fast commit lookups. I'll keep an eye out for that implementation.

> +	* {empty}
> +	offset (8 byte integer, network byte order): ::
> +	The offset from which that commit's bitmap can be read.
> +
> +	* {empty}
> +	xor offset (4 byte integer, network byte order): ::
> +	It holds the position of the triplet with whose bitmap the
> +	current bitmap need to xor. If the current triplet's bitmap
> +	do not have any xor bitmap, it defaults to 0xffffffff.

This last sentence seems backward. Perhaps:

  If the value is 0xffffffff, then the current bitmap has no xor bitmap.

Thanks,
-Stolee