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.

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

Improve Arc and Rc documentation #93109

Merged
merged 2 commits into from
Jan 22, 2022
Merged

Improve Arc and Rc documentation #93109

merged 2 commits into from
Jan 22, 2022

Conversation

JakobDegen
Copy link
Contributor

This makes two changes (I can split the PR if necessary, but the changes are pretty small):

  1. A bunch of trait implementations claimed to be zero cost; however, they use the Arc<T>: From<Box<T>> impl which is definitely not free, especially for large dynamically sized T.
  2. The code in deferred initialization examples unnecessarily used excessive amounts of unsafe. This has been reduced.

A number of trait implementations incorrectly claimed to be zero cost.
@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Jan 20, 2022
@rust-log-analyzer

This comment has been minimized.

@JakobDegen
Copy link
Contributor Author

Apparently I was wrong when I thought I knew how to run the tests locally...

@m-ou-se
Copy link
Member

m-ou-se commented Jan 21, 2022

The code in deferred initialization examples unnecessarily used excessive amounts of unsafe. This has been reduced.

I suspect the author of that used get_mut_unchecked rather than get_mut, to avoid unnecssary checks and panicking code in .unwrap(): https://godbolt.org/z/zKczvTx47

(For Rc the compiler can easily optimize the check out, but that (currently) doesn't work with Arc once atomics are involved.)

In general, the new_uninit+get_mut[_unchecked] api is a bit awkward, as it always involves a second function call which either requires unsafe or unwrap. There's some discussion on the tracking issue about a nicer interface: #63291 (comment)

Anyway, I agree that trading unsafe for unwrap is reasonable in documentation.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 21, 2022

📌 Commit 4de7618 has been approved by m-ou-se

@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 Jan 21, 2022
@JakobDegen
Copy link
Contributor Author

@m-ou-se oh yeah, I understood the point of doing it that way, but this is also sort of a separate issue in that deferred initialization within Arcs is not on its own a performance optimization but sometimes just the nicest way to write the thing; the .get_mut_unchecked() on the other hand is definitely a performance optimization, and there's no need to add it in many cases. (thanks @Nemo157 for originally pointing this out)

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#91965 (Add more granular `--exclude` in `x.py`)
 - rust-lang#92467 (Ensure that early-bound function lifetimes are always 'local')
 - rust-lang#92586 (Set the allocation MIN_ALIGN for espidf to 4.)
 - rust-lang#92835 (Improve error message for key="value" cfg arguments.)
 - rust-lang#92843 (Improve string concatenation suggestion)
 - rust-lang#92963 (Implement tuple array diagnostic)
 - rust-lang#93046 (Use let_else in even more places)
 - rust-lang#93109 (Improve `Arc` and `Rc` documentation)
 - rust-lang#93134 (delete `Stdin::split` forwarder)
 - rust-lang#93139 (rustdoc: fix overflow-wrap for table layouts)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9474c74 into rust-lang:master Jan 22, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 22, 2022
@JakobDegen JakobDegen deleted the arc-docs branch January 27, 2022 17:43
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants