Jeff King <peff@xxxxxxxx> writes:
> On Thu, Oct 31, 2024 at 10:39:51AM +0100, Karthik Nayak wrote:
>
>> @@ -652,20 +688,25 @@ unsigned char *use_pack(struct packed_git *p,
>> break;
>> }
>> if (!win) {
>> - size_t window_align = packed_git_window_size / 2;
>> + struct packfile_config config = PACKFILE_CONFIG_INIT;
>> + size_t window_align;
>> off_t len;
>>
>> + repo_config(p->repo, packfile_config, &config);
>> + window_align = config.packed_git_window_size / 2;
>> +
>
> Parsing config like this is somewhat expensive (remember we're going to
> hit your callback for every single config key in the system, user, and
> repo-level config files).
>
> And use_pack() is a relatively hot code path, as we call it any time we
> need to access bytes from a mapped pack! This "!win" conditional isn't
> quite as hot, as it only triggers when we establish a new window for a
> pack. But that still happens at least once per pack, more if we need to
> move the window around in a big pack, and lots more if we are under
> memory pressure and need to open/close windows a lot.
>
I must admit, I'm not too aware of the pack objects code base, but that
was my assumption indeed, that this conditional wasn't the hot path. But
even once per pack seems like quite the regression then.
> I think we need to parse these values once and then store them somewhere
> with cheaper access. Can we grab them in prepare_repo_settings(), for
> example, which would cache them? We need a repo struct, but we have one
> (the same packed_git->repo you are using to call repo_config()).
>
> -Peff
This seems like a good idea, I will amend this commit to move the config
to `repo_settings`. I think the previous commit doesn't require any
changes and can stay.
Thanks
- Karthik
Attachment:
signature.asc
Description: PGP signature
