On Fri, Nov 01, 2024 at 01:45:47PM -0400, Jeff King wrote:
> 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 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()).
Oh, wow, I can't believe that I missed this in my earlier reviews. Yes,
we should definitely *not* be calling an expensive function which
computes the same value every time in a hot path like 'use_pack()'.
Thanks for spotting.
Thanks,
Taylor
