-
Notifications
You must be signed in to change notification settings - Fork 13.7k
std: optimize dlsym! macro and add a test for it
#146019
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
Conversation
dlsym! macro and add a test for it (追記ここまで)
This comment has been minimized.
This comment has been minimized.
3aee6a7 to
0804417
Compare
Just curious...
@bors2 try @rust-timer queue
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
std: optimize `dlsym!` macro and add a test for it
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e2bfd7f): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 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 -1.0%, secondary -1.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 1.9%, secondary 2.2%)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: 467.034s -> 466.989s (-0.01%) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, r=me with a squash
* Ensure nul-termination of the symbol name at compile-time * Use an acquire load instead of a relaxed load and acquire fence * Properly use `unsafe` and add safety comments * Add tests
0332a66 to
d0a2761
Compare
std: optimize `dlsym!` macro and add a test for it The `dlsym!` macro always ensures that the name string is nul-terminated, so there is no need to perform the check at runtime. Also, acquire loads are generally faster than a load and a barrier, so use them. This is only false in the case where the symbol is missing, but that shouldn't matter too much.
The job test-various failed! Check out the build log: (web) (plain enhanced) (plain)
Click to see the possible cause of the failure (guessed by this bot)
failures:
---- sys::pal::unix::weak::tests::dlsym_existing stdout ----
thread 'sys::pal::unix::weak::tests::dlsym_existing' (515653) panicked at library/std/src/sys/pal/unix/weak/tests.rs:16:25:
called `Option::unwrap()` on a `None` value
stack backtrace:
0: __rustc::rust_begin_unwind
1: core::panicking::panic_fmt
2: core::panicking::panic
3: core::option::unwrap_failed
4: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
---- sys::pal::unix::weak::tests::dlsym_existing stdout end ----
failures:
sys::pal::unix::weak::tests::dlsym_existing
test result: FAILED. 542 passed; 1 failed; 1 ignored; 0 measured; 0 filtered out; finished in 3.54s
💔 Test failed - checks-actions
I wonder if it failed because of abs itself being a weak symbol so not getting pulled in if there is hardware support? Maybe something like printf would work better.
@rustbot author (aggressively making sure my assigned PRs are triaged today)
The
dlsym!macro always ensures that the name string is nul-terminated, so there is no need to perform the check at runtime. Also, acquire loads are generally faster than a load and a barrier, so use them. This is only false in the case where the symbol is missing, but that shouldn't matter too much.