Skip to content

Conversation

azhogin
Copy link
Contributor

@azhogin azhogin commented Oct 5, 2025

Continuation of #146880 and #147232.
'hir_owner_parent' query renamed 'hir_owner_parent_q'. hir_owner_parent inlined function added to optimize performance in case of non-incremental build.

'hir_owner_parent' query has low normalized average execution time (163ns) and good cache_hits (5773) according Daria's processed statistics. 'source_span', for comparison, has avg_ns_norm = 66ns and cache_hits = 11361.

Optimization may be profitable for queries with low normalized average execution time (to replace cache lookup into inlined call) and be significant with good cache_hits.

Query cache_hits min_ns max_ns avg_ns_norm
source_span 11361 18 2991 66
hir_owner_parent 5773 52 1773 163
is_doc_hidden 3134 47 1111 285
lookup_deprecation_entry 13905 36 6208 287
object_lifetime_default 5840 63 4688 290
upvars_mentioned 2575 75 7722 322
intrinsic_raw 21235 73 3453 367

Draft PR to measure performance changes.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 5, 2025
@Kobzol
Copy link
Member

Kobzol commented Oct 6, 2025

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Oct 6, 2025
hir_owner_parent optimized to inlined call for non-incremental build
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 6, 2025
@rust-bors
Copy link

rust-bors bot commented Oct 6, 2025

☀️ Try build successful (CI)
Build commit: f98c273 (f98c27362fe04fb22b3c3db681405edf9f642465, parent: 828c2a9afccf3b3ff8133368cfbc8bfe526aaa4d)

@rust-timer

This comment has been minimized.

@azhogin
Copy link
Contributor Author

azhogin commented Oct 6, 2025

r? @petrochenkov

@azhogin
Copy link
Contributor Author

azhogin commented Oct 6, 2025

r? @petrochenkov

@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2025

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

@azhogin
Copy link
Contributor Author

azhogin commented Oct 6, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 6, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f98c273): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.0% [0.0%, 0.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.5% [-1.1%, -0.1%] 10
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -1.0%, secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.3% [2.2%, 2.3%] 2
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
-1.9% [-2.3%, -1.6%] 2
All ❌✅ (primary) -1.0% [-1.0%, -1.0%] 1

Cycles

Results (secondary 1.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
5.3% [5.3%, 5.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.7% [-2.7%, -2.7%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 469.991s -> 471.779s (0.38%)
Artifact size: 388.38 MiB -> 388.40 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 6, 2025
@cjgillot
Copy link
Contributor

cjgillot commented Oct 6, 2025

You posted several PRs with this same idea. Instead of trying all queries, is there a way to address this structurally? I mean: change a macro in the query system to do this automatically?

@petrochenkov
Copy link
Contributor

I mean: change a macro in the query system to do this automatically?

That was the plan if the optimization worked.
It didn't work previously, so this PR and #147388 were going to be the last two attempts, but it seems like in this case it may actually work.

@petrochenkov
Copy link
Contributor

@azhogin
Could you write a bit of background in the PR descriptions? It's not just me who is reading them.
Mention that it's a continuation of #146880 and #147232, what is the idea behind the optimization, how is this query different from queries from the previous attempts, etc.

.copied()
.unwrap_or(ItemLocalId::ZERO),
}
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you factor this into an inline function instead of copypasting?

@petrochenkov
Copy link
Contributor

but it seems like in this case it may actually work.

I wonder why it works though.
The logic inside the provider function doesn't look obviously cheaper than a query cache lookup.
And if it's actually cheaper, then why didn't the previous PRs work. Perhaps the hir_owner_parent query is just much hotter?

change a macro in the query system to do this automatically?

A drawback in this is that you'll probably need a function pointer, and it won't be possible to replace a query call with an inlined cheap function call.
In any case, adding a macro probably doesn't make sense until there are at least several queries on which this optimization works.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants