Improve perf measurements of build_extern_trait_impl

[camelid]

Before, it was only measuring one callsite of build_impl, and it
incremented the call count even if build_impl returned early because
the did was already inlined.

Now, it measures all calls, minus calls that return early.

[rust-highfive]

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

[camelid]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 37e56fa8115203f422fe7fef84fa285a3a072e20 with merge c759bedb91d5b1f6fe5cd357d5b17e6141d6485f...

[camelid]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit eb713d2 with merge e9fd48d4c67a3126ae9b2d3bc6ad81753457c689...

[bors]

☀️ Try build successful - checks-actions
Build commit: e9fd48d4c67a3126ae9b2d3bc6ad81753457c689 (e9fd48d4c67a3126ae9b2d3bc6ad81753457c689)

[rust-timer]

Queued e9fd48d4c67a3126ae9b2d3bc6ad81753457c689 with parent 4e0d397, future comparison URL.

[rust-timer]

Finished benchmarking commit (e9fd48d4c67a3126ae9b2d3bc6ad81753457c689): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

[jyn514]

Hmm, when I added this it was intentional to only measure collect_trait_impls, since that's the hottest loop, and .insert already has some overhead I think it's important to measure. What other call sites are there for build_trait?

[camelid]

Hmm, when I added this it was intentional to only measure collect_trait_impls, since that's the hottest loop, and .insert already has some overhead I think it's important to measure. What other call sites are there for build_trait?

src/librustdoc/clean/utils.rs
194:                inline::build_impl(cx, None, did, None, ret);

src/librustdoc/clean/inline.rs
295:        build_impl(cx, parent_module, did, attrs, ret);

src/librustdoc/passes/collect_trait_impls.rs
35:                inline::build_impl(cx, None, did, None, &mut new_items);
44:                inline::build_impl(cx, None, def_id, None, &mut new_items);
117:                inline::build_impl(cx, None, impl_did, Some(&extra_attrs), &mut new_items);

[camelid]

Hmm, when I added this it was intentional to only measure collect_trait_impls, since that's the hottest loop

What would the downside be of measuring all uses though?

.insert already has some overhead I think it's important to measure

I would think the overhead of .insert is probably negligible though. Also, having those measurements is not really actionable, and IMO the improved accuracy of the execution count is more helpful than measuring the likely small amount of time .insert takes.

[jyn514]

Sure, I mean, at the end of the day anything this detailed will probably want to use cachegrind instead. I'm find with either landing this or not.

[camelid]

Sure, I mean, at the end of the day anything this detailed will probably want to use cachegrind instead. I'm find with either landing this or not.

The two reasons I'm proposing this change are (1) I want more accurate execution count numbers and (2) IMO it's more consistent to measure all calls of build_impl, not just a callsite that happens to be hot. I'm planning to at some point look into using cachegrind or a similar tool to investigate perf too :)

[jyn514]

@bors r+ rollup=never

[bors]

📌 Commit eb713d2 has been approved by jyn514

[bors]

⌛ Testing commit eb713d2 with merge a9f664f...

[bors]

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing a9f664f to master...

[rust-timer]

Finished benchmarking commit (a9f664f): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression