Re: [PATCH 1/7] odb/source-packed: improve lookup when enumerating objects

[Justin Tobler <jltobler@gmail.com>]

On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> When iterating through packed objects via `odb_for_each_object()` we
> do so via two different mechanisms:
> 
>   - When a multi-pack index is available we use that one to efficiently
>     loop through all objects.
> 
>   - We then loop through all packfiles that aren't covered by a
>     multi-pack index.

To be specific, we are talking only about the for_each_object callback
for the packed source `odb_source_packed_for_each_object()` correct?
Also, this appears to only matter when we are enumerating OIDs with a
specific prefix.

> Regardless of which mechanism we use, we then iterate through all the
> objects indexed by the respective data structure. Curiously though,
> while we use the indices for enumerating the objects, we completely
> ignore it for the actual object lookup. Instead, we call into the
> generic `odb_source_read_object_info()` function, which will itself
> consult the indices to figure out where the object in question even
> lives.
> 
> This has two consequences:
> 
>   - It's inefficient, as we basically have to figure out the position of
>     the object a second time.

Since we already have the position from the index, there is no need to
start over. Makes sense.

>   - It's subtly wrong, as it may now happen that a specific object will
>     be looked up via a different pack in case it exists multiple times.

Naive question: Is there any real harm in reading the same object, but
from a different packfile here?

Regardless I do think it's a good idea to just reuse the same packfile
to get the same object here.

> Fix the issue by using `packed_object_info()` directly. While at it,
> rename the `store` variable to `source`.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source-packed.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 0edea5356d..9cfa02b7a2 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -143,7 +143,7 @@ static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_f
>  }
>  
>  static int for_each_prefixed_object_in_midx(
> -	struct odb_source_packed *store,
> +	struct odb_source_packed *source,
>  	struct multi_pack_index *m,
>  	const struct odb_for_each_object_options *opts,
>  	struct odb_source_packed_for_each_object_wrapper_data *data)
> @@ -170,6 +170,7 @@ static int for_each_prefixed_object_in_midx(
>  		 */
>  		for (i = first; i < num; i++) {
>  			const struct object_id *current = NULL;
> +			struct packed_git *pack;
>  			struct object_id oid;
>  
>  			current = nth_midxed_object_oid(&oid, m, i);
> @@ -177,9 +178,8 @@ static int for_each_prefixed_object_in_midx(
>  			if (!match_hash(len, opts->prefix->hash, current->hash))
>  				break;
>  
> -			if (opts->flags) {
> +			if (opts->flags || data->request) {

I'm not sure I follow why the above condition needed to change.

>  				uint32_t pack_id = nth_midxed_pack_int_id(m, i);
> -				struct packed_git *pack;
>  
>  				if (prepare_midx_pack(m, pack_id)) {
>  					pack_errors = true;
> @@ -193,9 +193,9 @@ static int for_each_prefixed_object_in_midx(
>  
>  			if (data->request) {
>  				struct object_info oi = *data->request;
> +				off_t offset = nth_midxed_offset(m, i);
>  
> -				ret = odb_source_read_object_info(&store->base, current,
> -								  &oi, 0);
> +				ret = packed_object_info(source, pack, offset, &oi);

We not longer use the generic function to read object info. This ensures
the exact same object is read.

>  				if (ret)
>  					goto out;
>  
> @@ -219,7 +219,7 @@ static int for_each_prefixed_object_in_midx(
>  }
>  
>  static int for_each_prefixed_object_in_pack(
> -	struct odb_source_packed *store,
> +	struct odb_source_packed *source,
>  	struct packed_git *p,
>  	const struct odb_for_each_object_options *opts,
>  	struct odb_source_packed_for_each_object_wrapper_data *data)
> @@ -246,8 +246,9 @@ static int for_each_prefixed_object_in_pack(
>  
>  		if (data->request) {
>  			struct object_info oi = *data->request;
> +			off_t offset = nth_packed_object_offset(p, i);
>  
> -			ret = odb_source_read_object_info(&store->base, &oid, &oi, 0);
> +			ret = packed_object_info(source, p, offset, &oi);

And we do the same thing here when reading the object from a packfile.

-Justin