Skip to content

Conversation

ChrisDenton
Copy link
Member

I've now come around to the conclusion that there is a justification for pre-loading the synchronization functions WaitOnAddress and WakeByAddressSingle. I've found this to have a particularly impact in testing frameworks that may have short lived processes which immediately spawn lots of threads.

Also, because pre-main initializers imply a single-threaded environment, we can switch back to using relaxed atomics which might be a minor perf improvement on some platforms (though I doubt it's particularly notable).

r? @Mark-Simulacrum and sorry for the churn here.

For convenience I'll summarise previous issues with preloading and the solutions that are included in this PR (if any):

Issue: User pre-main initializers may be run before std's
Solution: The std now uses initializers that are guaranteed to run earlier than the old initializers. A note is also added that users should not copy std's behaviour if they want to ensure they run their initializers after std.

Issue: Miri does not understand pre-main initializers.
Solution: For miri only, run the function loading lazily instead.

Issue: We should ideally use LoadLibrary to get "api-ms-win-core-synch-l1-2-0". Only "ntdll" and "kernel32" are guaranteed to always be loaded.
Solution: None. We can't use LoadLibrary pre-main. However, in the past GetModuleHandle has always worked in practice so this should hopefully not be a problem.

If/when Windows 7 support is dropped, we can finally remove all this for good and just use normal imports.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 25, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2022
@Mark-Simulacrum Mark-Simulacrum 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 Aug 27, 2022
@ChrisDenton
Copy link
Member Author

I've updated the comments to hopefully be a bit more accurate.

@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 Aug 28, 2022
@Mark-Simulacrum
Copy link
Member

r=me with commits squashed.

@Mark-Simulacrum Mark-Simulacrum 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 Aug 28, 2022
@ChrisDenton
Copy link
Member Author

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Aug 28, 2022

📌 Commit 7bb47a6 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 31, 2022
…-Simulacrum

Reinstate preloading of some dll imports

I've now come around to the conclusion that there is a justification for pre-loading the synchronization functions `WaitOnAddress` and `WakeByAddressSingle`. I've found this to have a particularly impact in testing frameworks that may have short lived processes which immediately spawn lots of threads.

Also, because pre-main initializers imply a single-threaded environment, we can switch back to using relaxed atomics which might be a minor perf improvement on some platforms (though I doubt it's particularly notable).

r? `@Mark-Simulacrum` and sorry for the churn here.

For convenience I'll summarise previous issues with preloading and the solutions that are included in this PR (if any):

**Issue:** User pre-main initializers may be run before std's
**Solution:** The std now uses initializers that are guaranteed to run earlier than the old initializers. A note is also added that users should not copy std's behaviour if they want to ensure they run their initializers after std.

**Issue:** Miri does not understand pre-main initializers.
**Solution:** For miri only, run the function loading lazily instead.

**Issue:** We should ideally use `LoadLibrary` to get "api-ms-win-core-synch-l1-2-0". Only "ntdll" and "kernel32" are guaranteed to always be loaded.
**Solution:** None. We can't use `LoadLibrary` pre-main. However, in the past `GetModuleHandle` has always worked in practice so this should hopefully not be a problem.

If/when Windows 7 support is dropped, we can finally remove all this for good and just use normal imports.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2022
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#100970 (Allow deriving multipart suggestions)
 - rust-lang#100984 (Reinstate preloading of some dll imports)
 - rust-lang#101011 (Use getentropy when possible on all Apple platforms)
 - rust-lang#101025 (Add tier-3 support for powerpc64 and riscv64 openbsd)
 - rust-lang#101049 (Remove span fatal from ast lowering)
 - rust-lang#101100 (Make call suggestions more general and more accurate)
 - rust-lang#101171 (Fix UB from misalignment and provenance widening in `std::sys::windows`)
 - rust-lang#101185 (Tweak `WellFormedLoc`s a bit)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b2a8d9d into rust-lang:master Aug 31, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 31, 2022
@ChrisDenton ChrisDenton deleted the reinstate-init branch August 31, 2022 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants