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

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

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

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