-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[Experiment] Do not emit noalias for some functions including .as_mut_ptr()
#146823
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
base: master
Are you sure you want to change the base?
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
[Experiment] Do not emit noalias for inline functions
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (b1487a9): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.2%, secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 0.9%, secondary 1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 472.936s -> 470.008s (-0.62%) |
That result definitely indicates some kind of missed optimization in LLVM, Overall, the above report reminds me this perf experiment I did a little while ago: #121068 (comment) (basically the same result, check builds are slower which means less optimization of the compiler, but the hashmap runtime benchmarks benefit) |
Sigh. So we can't attach that meaning to the Now that we are here, let's test it with |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[Experiment] Do not emit noalias for inline functions
This comment has been minimized.
This comment has been minimized.
LLVM has scoped noalias metadata, which is supposed to represent noalias information from a function that got inlined. So I don't think it's completely accurate to say that we expect @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
[Experiment] Do not emit noalias for inline functions
@bors try- |
I didn't notice you successfully did a try build. This should be faster. @rust-timer build f1987df |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (f1987df): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.2%, secondary -0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 472.936s -> 473.189s (0.05%) |
Is this result statistically significant? The regressed example didn't appear in the previous one. Anyway, I lost my interest in Here is my new idea: instead of disabling |
.as_mut_ptr()
The instruction count differences are in optimized builds, which probably means that the impact of generating less work for LLVM dominated in those cases. Whether they are statistically significant doesn't matter because of which scenarios reported a change.
You cannot find such functions via text search. Retags recurse into structures, and in our current implementation, if your type wraps a reference but is passed as Scalar or ScalarPair, its pointer still gets My bigger concern with this approach is that you are trying to carve out "functions like slice::as_mut_ptr" by just looking at the signature when the problem is a combination of the signature (that it takes a reference) and the implementation (all the function does is convert the reference to a raw pointer).
It can't hurt to have run the experiment; if it turns up no effect that doesn't necessarily mean anything, but if it turns up a negative effect we can discard this line of reasoning :p |
@bors try |
This comment has been minimized.
This comment has been minimized.
[Experiment] Do not emit noalias for some functions including `.as_mut_ptr()`
I think looking at the implementation in the model is hard (hard to find good properties, hard to implement, and hard to reason about for users). Maybe there is no easy solution, but let's try. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@rust-timer build 8839836 |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (8839836): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.3%, secondary -0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 471.863s -> 471.5s (-0.08%) |
r? @saethlin