Re: [PATCH] pack-objects: no crc check when the cached version is used
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 13, 2013, 18:28 UTC
- Message-ID
- <xmqq7gekk24q.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1379070180-15947-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 7 quoted lines
> Current code makes pack-objects always do check_pack_crc() in > unpack_entry() even if right after that we find out there's a cached > version and pack access is not needed. Swap two code blocks, search > for cached version first, then check crc. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > ---
Interesting.
This is only triggered inside pack-objects, which would read a lot of data from existing packs, and the overhead for looking up the entry from the revindex, faulting in the actual packdata, and computing and comparing the crc would not be trivial, especially as the cost is incurred over many objects we need to untangle in the delta chain. If you have interesting numbers to show how much this improves the performance, I am curious to see it.
Good spotting ;-)
Show 41 quoted lines
> sha1_file.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 8c2d1ed..4955724 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2126,6 +2126,16 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
> int i;
> struct delta_base_cache_entry *ent;
>
> + ent = get_delta_base_cache_entry(p, curpos);
> + if (eq_delta_base_cache_entry(ent, p, curpos)) {
> + type = ent->type;
> + data = ent->data;
> + size = ent->size;
> + clear_delta_base_cache_entry(ent);
> + base_from_cache = 1;
> + break;
> + }
> +
> if (do_check_packed_object_crc && p->index_version > 1) {
> struct revindex_entry *revidx = find_pack_revindex(p, obj_offset);
> unsigned long len = revidx[1].offset - obj_offset;
> @@ -2140,16 +2150,6 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
> }
> }
>
> - ent = get_delta_base_cache_entry(p, curpos);
> - if (eq_delta_base_cache_entry(ent, p, curpos)) {
> - type = ent->type;
> - data = ent->data;
> - size = ent->size;
> - clear_delta_base_cache_entry(ent);
> - base_from_cache = 1;
> - break;
> - }
> -
> type = unpack_object_header(p, &w_curs, &curpos, &size);
> if (type != OBJ_OFS_DELTA && type != OBJ_REF_DELTA)
> break;