Re: [PATCH] perf record: Cache build-ID of hit DSOs only

[Namhyung Kim <namhyung@kernel.org>]

Hi Arnaldo,

On Thu, Jul 31, 2025 at 11:27:57AM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Jul 31, 2025 at 12:03:30AM -0700, Namhyung Kim wrote:
> > It post-processes samples to find which DSO has samples.  Based on that
> > info, it can save used DSOs in the build-ID cache directory.  But for
> > some reason, it saves all DSOs without checking the hit mark.  Skipping
> > unused DSOs can give some speedup especially with --buildid-mmap being
> > default.
>  
> > On my idle machine, `time perf record -a sleep 1` goes down from 3 sec
> > to 1.5 sec with this change.
> 
> Good stuff, and this is in line with the original intent, don't cache
> uninteresting things.
> 
> But now I have do some digging, as this should've been the case since
> the start, why would we go to the trouble of traversing perf.data,
> processing all samples, yadda, yadda to then not look at it when caching
> files?
> 
> The whole process of reading the build ids at the end is done here:
> 
> bool dsos__read_build_ids(struct dsos *dsos, bool with_hits)
> {
>         struct dsos__read_build_ids_cb_args args = {
>                 .with_hits = with_hits,
>                 .have_build_id = false,
>         };
> 
>         dsos__for_each_dso(dsos, dsos__read_build_ids_cb, &args);
>         return args.have_build_id;
> }
> 
> And that dsos__read_build_ids_cb thing specifically looks at:
> 
> static int dsos__read_build_ids_cb(struct dso *dso, void *data)
> {
>         struct dsos__read_build_ids_cb_args *args = data;
>         struct nscookie nsc;
>         struct build_id bid = { .size = 0, };
>                                 
>         if (args->with_hits && !dso__hit(dso) && !dso__is_vdso(dso))
>                 return 0;
> <SNIP>
> 
> So it will not try to read the build id if that DSO has no samples.
> 
> But, that was written before PERF_RECORD_MMAP* came with a build-id, so
> it _will_ have a build-id and thus checking if it has hits is needed.
> 
> In the past DSOs without hits wouldn't have a build-id because
> dsos__read_build_ids_cb() would not read that build-id from the ELF
> file.

Yep, I think that's the reason.

> 
> Ok, now that makes sense:
> 
> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Thanks!

> 
> This could have a Fixes attached to it, one that doesn't fixes something
> that is not working, but speeds up processing by overcoming an oversight
> when adding build-ids to MMAP records, so I think a:
> 
> Fixes: e29386c8f7d71fa5 ("perf record: Add --buildid-mmap option to enable PERF_RECORD_MMAP2's build id")
> 
> Is worth.

Sure, will add.

Thanks,
Namhyung