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

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

[Namhyung Kim]

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.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/build-id.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index e2b295fe4d2fe552..a7018a3b0437a699 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -872,7 +872,7 @@ static int dso__cache_build_id(struct dso *dso, struct machine *machine,
 	char *allocated_name = NULL;
 	int ret = 0;
 
-	if (!dso__has_build_id(dso))
+	if (!dso__has_build_id(dso) || !dso__hit(dso))
 		return 0;
 
 	if (dso__is_kcore(dso)) {
-- 
2.50.1

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

[Arnaldo Carvalho de Melo]

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.

Ok, now that makes sense:

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

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.

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/build-id.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index e2b295fe4d2fe552..a7018a3b0437a699 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -872,7 +872,7 @@ static int dso__cache_build_id(struct dso *dso, struct machine *machine,
>  	char *allocated_name = NULL;
>  	int ret = 0;
>  
> -	if (!dso__has_build_id(dso))
> +	if (!dso__has_build_id(dso) || !dso__hit(dso))
>  		return 0;
>  
>  	if (dso__is_kcore(dso)) {
> -- 
> 2.50.1

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

[Namhyung Kim]

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

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

[Namhyung Kim]

Hi Ian,

On Thu, Jul 31, 2025 at 5:10 PM Ian Rogers <irogers@google.com> wrote:
>
>
>
> On Fri, Aug 1, 2025, 1:29 AM Namhyung Kim <namhyung@kernel.org> wrote:
>>
>> 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.
>
>
> Sorry for the issue, I hope the market dsos is correct. I'm not sure the fixes is correct as that patch called out not populating the .debug. we should make sure that's still the case for people already using --buildid-mmap, they shouldn't have the .debug filled in. Sorry for my lack of a plain text email.

That's guarded by rec->no_buildid so it shouldn't change the behavior.

Thanks,
Namhyung

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

[Namhyung Kim]

On Thu, 31 Jul 2025 00:03:30 -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.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung