Re: [PATCH v2 04/10] packfile: inline cache_or_unpack_entry
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 26, 2024, 17:09 UTC
- Message-ID
- <xmqqjzg3ky77.fsf@gitster.g>
- In-Reply-To
- <20240823224630.1180772-5-e@80x24.org>
Eric Wong <e@80x24.org> writes:
Show 8 quoted lines
> We need to check delta_base_cache anyways to fill in the > `whence' field in `struct object_info'. Inlining (and getting > rid of) cache_or_unpack_entry() makes it easier to only do the > hashmap lookup once and avoid a redundant lookup later on. > > This code reorganization will also make an optimization to > use the cache entry directly easier to implement in the next > commit.
"cache entry" -> "cached entry"; we tend to use "cache entry" exclusively to mean an entry in the in-core index structure, and not the cached objects held in the object layer.
Show 18 quoted lines
> {
> struct pack_window *w_curs = NULL;
> - unsigned long size;
> off_t curpos = obj_offset;
> enum object_type type;
> + struct delta_base_cache_entry *ent;
>
> /*
> * We always get the representation type, but only convert it to
> * a "real" type later if the caller is interested.
> */
> - if (oi->contentp && !oi->content_limit) {
> - *oi->contentp = cache_or_unpack_entry(r, p, obj_offset, oi->sizep,
> - &type);
> + oi->whence = OI_PACKED;
> + ent = get_delta_base_cache_entry(p, obj_offset);
> + if (ent) {
> + oi->whence = OI_DBCACHED;OK. This is very straight-forward. It is packed but if we grabbed it from the delta-base-cache, that is the only case we know it is dbcached.
Show 9 quoted lines
> + type = ent->type;
> + if (oi->sizep)
> + *oi->sizep = ent->size;
> + if (oi->contentp) {
> + if (!oi->content_limit ||
> + ent->size <= oi->content_limit)
> + *oi->contentp = xmemdupz(ent->data, ent->size);
> + else
> + *oi->contentp = NULL; /* caller must stream */This assignment of NULL is more explicit than the original; is it because the original assumed that *(oi->contentp) is initialized to NULL if oi->contentp asks us to give the contents?
Show 5 quoted lines
> + } else if (oi->contentp && !oi->content_limit) {
> + *oi->contentp = unpack_entry(r, p, obj_offset, &type,
> + oi->sizep);
> if (!*oi->contentp)
> type = OBJ_BAD;Nice. The code structure is still easy to follow, even though the if/else cascade here are organized differently with more cases (used to be "are we peeking the contents, or not?"---now it is "do this if we can grab from the delta base cache, do one of these other things if we have go to the packfile").