Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cache-based fast path for cfgs and check-cfgs #122207

Closed
wants to merge 2 commits into from

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Mar 8, 2024

This PR add a fast path for testing if a cfg is enabled and is expected.

This cache reduce the number of lookup from 2/3 to just 1 (in the good path).

Currently:

  • when there are no expecteds cfgs, we do 2 lookup (one for finding if the cfg is enabled and the second for trying to get the expected values)
  • when there are some expecteds cfgs, we do 3 lookup in the good case (same as above + one to see if the value is in the expected values) and 2 in the "bad" case

With this PR we will only do 1 lookup if the cfg is expected (aka the good case) no matter if the cfg is actually enabled or not. See the PR changes/commits for more details.

The perf runs done below shows that there is no to a slightly improvement, which is expected as we do less work, but we also don't have any benchmark with many cfgs to be assertive.

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 8, 2024
@Urgau

This comment was marked as outdated.

@matthiaskrgr
Copy link
Member

@bors try @rust-timer queue

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2024

Failed to set assignee to ghost: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 8, 2024
@Urgau

This comment was marked as resolved.

@rustbot

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
Add cache-based fast path for cfgs and check-cfgs

TODO, waiting for perf.
@bors
Copy link
Contributor

bors commented Mar 8, 2024

⌛ Trying commit 24d06a9 with merge 4ee97b1...

@workingjubilee
Copy link
Member

@Urgau I believe the handling specially-recognizes r? @ghost in the PR opening.

In any case, it's somewhat useless after-the-fact, as they will remain subscribed to the thread and must manually unsubscribe.

@Urgau
Copy link
Member Author

Urgau commented Mar 8, 2024

Yeah, I kinda knew about it but I was hoping I was wrong 😄, but the real issue is that rustbot didn't considered the Draft status and assigned someone anyway. It should have waited for the status to changed; but that's a topic for another time.

@bors
Copy link
Contributor

bors commented Mar 8, 2024

☀️ Try build successful - checks-actions
Build commit: 4ee97b1 (4ee97b1068420a05fa2f91425b1a5f83f240767f)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4ee97b1): comparison URL.

Overall result: ✅ improvements - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.8% [1.2%, 2.1%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.7% [-4.9%, -2.5%] 2
All ❌✅ (primary) - - 0

Cycles

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

Binary size

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

Bootstrap: 649.894s -> 649.476s (-0.06%)
Artifact size: 172.55 MiB -> 172.57 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 9, 2024
@workingjubilee
Copy link
Member

hot unambiguous perf results!

@Urgau
Copy link
Member Author

Urgau commented Mar 9, 2024

No perf improvement or regression, which is expected since don't have any check-cfg or big cfg test.

Let's try an approach that includes all expected cfgs and cfg. It again shouldn't have any impact but it's switching from FxHashSet to FxHashMap so just to be sure may I get another perf run (cc @matthiaskrgr).

@matthiaskrgr
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 9, 2024
@bors
Copy link
Contributor

bors commented Mar 9, 2024

⌛ Trying commit b5e463d with merge 478862c...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 9, 2024
Add cache-based fast path for cfgs and check-cfgs

TODO, waiting for perf.
@bors
Copy link
Contributor

bors commented Mar 9, 2024

☀️ Try build successful - checks-actions
Build commit: 478862c (478862ce99721d8d99c698714ab15e5601c67673)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (478862c): comparison URL.

Overall result: ✅ improvements - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

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

Binary size

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

Bootstrap: 649.076s -> 653.276s (0.65%)
Artifact size: 172.58 MiB -> 310.43 MiB (79.87%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 9, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Mar 9, 2024

(The artifact size change is just getting the libLLVM.so size back, thanks to rust-lang/rustc-perf#1838).

@Urgau Urgau marked this pull request as ready for review March 9, 2024 20:00
@Urgau
Copy link
Member Author

Urgau commented Mar 9, 2024

r? @petrochenkov

@petrochenkov
Copy link
Contributor

This cache complicates the cfg logic, so I don't want to merge it until there are some benchmarks showing improvements.
Let's maybe block this on rust-lang/cargo#13571 reaching rustc-perf.
@rustbot blocked

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2024
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 6, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 6, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

@lqd
Copy link
Member

lqd commented May 6, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 6, 2024
@bors
Copy link
Contributor

bors commented May 6, 2024

⌛ Trying commit e6dcf99 with merge 81d5a42...

bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2024
Add cache-based fast path for cfgs and check-cfgs

This PR add a fast path for testing if a cfg is enabled and is expected.

This cache reduce the number of lookup from 2/3 to just 1 (in the good path).

Currently:
 - when there are no expecteds cfgs, we do 2 lookup (one for finding if the cfg is enabled and the second for trying to get the expected values)
 - when there are some expecteds cfgs, we do 3 lookup in the good case (same as above + one to see if the value is in the expected values) and 2 in the "bad" case

With this PR we will only do 1 lookup if the cfg is expected (aka the good case) no matter if the cfg is actually enabled or not. See the PR changes/commits for more details.

The perf runs done below shows that there is no to a slightly improvement, which is expected as we do less work, but we also don't have any benchmark with many cfgs to be assertive.
@bors
Copy link
Contributor

bors commented May 6, 2024

☀️ Try build successful - checks-actions
Build commit: 81d5a42 (81d5a42c668f851052b6dc31326831d39f9eba5f)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (81d5a42): comparison URL.

Overall result: ❌ regressions - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.6% [3.3%, 4.0%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 676.654s -> 676.922s (0.04%)
Artifact size: 315.91 MiB -> 315.91 MiB (0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 6, 2024
@Urgau
Copy link
Member Author

Urgau commented May 7, 2024

Nothing significant. Closing.

@Urgau Urgau closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-merge-commits PR has merge commits, merge with caution. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. 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.

10 participants