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.

Sign up for GitHub

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

Jump to bottom

libstd: remove some mutable statics in sys::unix #74006

Merged
merged 1 commit into from
Jul 7, 2020
Merged

libstd: remove some mutable statics in sys::unix #74006

merged 1 commit into from
Jul 7, 2020

Conversation

euclio
Copy link
Contributor

@euclio euclio commented Jul 3, 2020

My understanding is that this achieves the same behavior and performance with safe code.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2020
@joshtriplett
Copy link
Member

Could someone from @rust-lang/libs take a look at this?

SimonSapin
SimonSapin approved these changes Jul 5, 2020
View reviewed changes
Copy link
Contributor

@SimonSapin SimonSapin left a comment

Choose a reason for hiding this comment

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

Per https://blog.rust-lang.org/inside-rust/2020/07/02/Ownership-Std-Implementation.html I think @rust-lang/compiler could also handle this. Anyway, this looks good.

src/libstd/sys/unix/args.rs Outdated
.map(|i| {
let cstr = CStr::from_ptr(*ARGV.offset(i) as *const libc::c_char);
let cstr = CStr::from_ptr(
*ARGV.load(Ordering::Relaxed).offset(i) as *const libc::c_char
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make a difference to move this load out of the loop?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unlikely to make a perf difference, but I like it anyway, because it'll make it clear that there is no expectation of this loop to ever load a different value while looping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@dtolnay dtolnay added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 6, 2020
@SimonSapin
Copy link
Contributor

Looks good, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jul 6, 2020

📌 Commit 7d7f167 has been approved by SimonSapin

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 6, 2020
@Manishearth
Rollup merge of rust-lang#74006 - euclio:sys-unix-static-mut, r=Simon…
678949b 
…Sapin

libstd: remove some mutable statics in sys::unix

My understanding is that this achieves the same behavior and performance with safe code.
@Manishearth Manishearth mentioned this pull request Jul 6, 2020
Closed
@Manishearth
Copy link
Member

@bors r-

#74106 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 6, 2020
@euclio
Copy link
Contributor Author

euclio commented Jul 7, 2020

Fixed, should be ready for another run.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 7, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jul 7, 2020

📌 Commit 792f2de has been approved by oli-obk

@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 Jul 7, 2020
@bors
Copy link
Contributor

bors commented Jul 7, 2020

⌛ Testing commit 792f2de with merge 42108486723e0a2d7d03888673ca838dd2d19cde...

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 7, 2020
@Manishearth
Rollup merge of rust-lang#74006 - euclio:sys-unix-static-mut, r=oli-obk
f2a5068 
libstd: remove some mutable statics in sys::unix

My understanding is that this achieves the same behavior and performance with safe code.
@Manishearth Manishearth mentioned this pull request Jul 7, 2020
Closed
@Manishearth
Copy link
Member

@bors retry yield

@bors
Copy link
Contributor

bors commented Jul 7, 2020

⌛ Testing commit 792f2de with merge 8ac1525...

@RalfJung
Copy link
Member

RalfJung commented Jul 7, 2020 •
edited
Loading

My understanding is that this achieves the same behavior and performance with safe code.

Relaxed accesses are slightly stronger than non-atomic accesses that were used before, and optimized a bit less. (IOW, behavior is slightly different.) So in theory this could lead to slightly worse performance.

That seems unlikely in practice though, unless these are used in tight loops.

@bors
Copy link
Contributor

bors commented Jul 7, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 8ac1525 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 7, 2020
@bors bors merged commit 8ac1525 into rust-lang:master Jul 7, 2020
@euclio euclio deleted the sys-unix-static-mut branch July 8, 2020 00:49
@RalfJung
Copy link
Member

RalfJung commented Jul 8, 2020

This PR caused a regression: there are now aliasing violations in the argc/argv handling.

@RalfJung
Copy link
Member

RalfJung commented Jul 8, 2020

Ah, this is probably caused by the integer-ptr-casts that AtomicPtr is performing internally. :/

@RalfJung
Copy link
Member

RalfJung commented Jul 8, 2020

I'll have to fix this by changes on the Miri side until we can handle int-ptr-casts better.

@RalfJung RalfJung mentioned this pull request Jul 8, 2020
Merged
bors added a commit to rust-lang/miri that referenced this pull request Jul 8, 2020
@bors
Auto merge of #1470 - RalfJung:machine-tracking, r=RalfJung
2bea1da 
we cannot track all machine memory any more due to int-ptr-casts

Fixes a regression introduced by rust-lang/rust#74006
bors added a commit to rust-lang/miri that referenced this pull request Jul 8, 2020
@bors
Auto merge of #1470 - RalfJung:machine-tracking, r=RalfJung
578aebf 
we cannot track all machine memory any more due to int-ptr-casts

Fixes a regression introduced by rust-lang/rust#74006
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@SimonSapin SimonSapin SimonSapin approved these changes

Assignees

@joshtriplett joshtriplett

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.46
Development

Successfully merging this pull request may close these issues.
10 participants
@euclio @rust-highfive @joshtriplett @SimonSapin @bors @Manishearth @oli-obk @RalfJung @cuviper @dtolnay