Re: [PATCH v3 2/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 4, 2022, 22:09 UTC
Message-ID: <Y2WNnqOOu+1QaOPq@nand.local>
In-Reply-To: <7ac9b859f31b2b3efb4a04896892ccd094a98734.1667470481.git.dyroneteng@gmail.com>

On Fri, Nov 04, 2022 at 11:17:10AM +0800, Teng Long wrote:

Show 9 quoted lines

> From: Teng Long <dyroneteng@gmail.com>
>
> Everytime when calling "open_pack_bitmap_1()", we will firstly call
> "open_pack_index(packfile)" to check the index, then further check
> again in "is_pack_valid()" before mmap the bitmap file. So, let's
> remove the first check here.
>
> The relate discussion:
> https://public-inbox.org/git/Y2IiSU1L+bJPUioV@coredump.intra.peff.net/#t

The related discussion is good, but I would have liked to see some of the details make its way into the patch message for future readers.

Maybe something like:

    When trying to open a pack bitmap, we call open_pack_bitmap_1() in a
    loop, during which it tries to open up the pack index corresponding
    with each available pack.

    It's likely that we'll end up relying on objects in that pack later
    in the process (in which case we're doing the work of opening the
    pack index optimistically), but not guaranteed.

    For instance, consider a repository with a large number of small
    packs, and one large pack with a bitmap. If we see that bitmap pack
    last in our loop which calls open_pack_bitmap_1(), the current code
    will have opened *all* pack index files in the repository. If the
    request can be served out of the bitmapped pack alone, then the time
    spent opening these idx files was wasted.S

    Since open_pack_bitmap_1() calls is_pack_valid() later on (which in
    turns calls open_pack_index() itself), we can just drop the earlier
    call altogether.

Thanks, Taylor