Skip to content

Conversation

FractalFir
Copy link
Contributor

(Hopefully) fixes: #2008 on Linux.

Uses directories /sys/devices/ to detect if the system has a "Performace-Hybrid" architecture(has P and E cores). If P cores are detected, forces the benchmark to only run on Performance cores using taskset.

Changes the P-core specific perf events cpu_core/instructions:u/ to "normal" events instructions:u. I am a bit unsure if my implementation of that is correct, tough.

@FractalFir
Copy link
Contributor Author

It seems like this repo is using a fairly old version of Rust( 1.75.0 which is 1 year old) in its CI.

Is there a reason this can't be raised(eg. MSRV)? If so, I can adapt this patch to use older Rust.

@Kobzol
Copy link
Member

Kobzol commented Feb 11, 2025

It seems like this repo is using a fairly old version of Rust( 1.75.0 which is 1 year old) in its CI.

I like that Rust is bringing an "always up-to-date" philosophy, so that a year old version is considered to be "fairly old" 😆

I don't think that it's needed to stay on 1.75, feel free to update to a newer version, I'd say 1.82 the newest.

@Kobzol
Copy link
Member

Kobzol commented Feb 11, 2025

I don't have P/E cores, so I can't test this, but I assume that it works for you. @bjorn3 Since you created #2008, could you please test if this patch helps to make rustc-perf work on your P/E system?

@FractalFir
Copy link
Contributor Author

I don't have P/E cores, so I can't test this, but I assume that it works for you.

I think it works for me(I can see the profiling results), but I also don't know enough about rustc-perf to conform that the numbers are accurate.

@Kobzol
Copy link
Member

Kobzol commented Feb 11, 2025

Could you send some icount results for a few benchmarks? Just so that we can compare. And also the number of (P) cores that you have.

@lqd
Copy link
Member

lqd commented Feb 11, 2025

Could you also rustfmt your code please, as well as fixing the unused warnings? (there seem to be a few typos in there as well). Thanks.

@FractalFir
Copy link
Contributor Author

It looks like some of the CI failures are caused by code I did not touch. I don' think fixing them in this PR would be right - I can fix them in another PR if needed.

I think I caught all the typos in comments.

As for the icounts, I only have the ones for an experimental patch to rustc. They are different from the ones on the website, but that might just be caused by the patch in question. I guess I'll have to run a benchmark on the baseline compiler.

For summary-check they are(taken from the graph):
1.461 incr-full
1 full
0.887 incr-patched
0.387 incr-unchanged
For bitmaps-3.1.0-opt, they are roughly

10.81 G incr-full
7.64 G full
3.21 G incr-patched
2.66 G incr-unchanged

I have 8 P cores(12 if you count the ones from hyper-threading) .

@Kobzol
Copy link
Member

Kobzol commented Feb 12, 2025

Interesting. I have this:
image

With the stable compiler (1.84.1), on a PC with 8 cores. The ratios are similar, but the absolute counts are quite different.

@Kobzol
Copy link
Member

Kobzol commented Feb 12, 2025

The CI failures appeared because you bumped the toolchain, so clippy now has new issues with the code :) I'll update it to 1.81 in a separate PR.

@Kobzol Kobzol mentioned this pull request Feb 12, 2025
@Kobzol
Copy link
Member

Kobzol commented Feb 12, 2025

#2042

@Kobzol
Copy link
Member

Kobzol commented Feb 12, 2025

It would be better to rebase on top of master and then force-push, than to create the merge commit. You can check https://rustc-dev-guide.rust-lang.org/git.html#rebasing for some hints.

@FractalFir FractalFir force-pushed the master branch 2 times, most recently from 3972c8f to 075b2a9 Compare February 13, 2025 16:23
@FractalFir
Copy link
Contributor Author

FractalFir commented Feb 13, 2025

I fixed the suggestions made here:

  1. The entire thing is now properly rebased
  2. Added some comments explaining why E cores are not used.
  3. Added a warning message, informing the user that only P cores are used.
  4. CARGO_BUILD_JOBS is now set to the P core count, to reflect the restriction to only use P cores.

Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thanks! I appreciate that you took the time to fix this issue.

Since rustfmt choked on your code, I becometh rustfmt in the latest commit.

@Kobzol Kobzol merged commit 2477025 into rust-lang:master Feb 13, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not working well with systems that use P and E cores
4 participants