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

Avoid unnecessary synchronization in {force,deref}_mut #231

Merged
merged 4 commits into from
May 29, 2023

Conversation

danielhenrymantilla
Copy link
Contributor

  • DerefMut was not delegating to force_mut() (contary to the by-ref APIs);
  • Lazy::force_mut was not taking advantage of exclusive-mutability access, and instead paying shared-mutability access. Mainly, in the sync case, it involved atomic operations which are now skipped.

Fixes #226

  - `DerefMut` was not delegating to `force_mut()` (contary to the
    by-ref APIs);
  - `Lazy::force_mut` was not taking advantage of exclusive-mutability
    access, and instead paying shared-mutability access. Mainly, in the
    `sync` case, it involved atomic operations which are now skipped.

Fixes matklad#226
};
this.cell = OnceCell::with_value(value);
}
this.cell.get_mut().unwrap_or_else(|| unreachable!())
Copy link
Contributor Author

@danielhenrymantilla danielhenrymantilla May 28, 2023

Choose a reason for hiding this comment

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

(Trying to rewrite this so as to avoiding the double-get_mut() on the happy path runs into polonius)

@matklad
Copy link
Owner

matklad commented May 28, 2023

Could you bump a patch version in Cargo.toml and add a short note to the Changelog?

@matklad
Copy link
Owner

matklad commented May 28, 2023

bors d+

@bors
Copy link
Contributor

bors bot commented May 28, 2023

✌️ danielhenrymantilla can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented May 28, 2023

@matklad I won't approve the PR myself since I have also taken the liberty to add a Cargo.lock.msrv check (while also updating it), which you may want to review 🙂

  • (I'll admit I could have opened another PR for that xtask tweak commit; I'm taking a shortcut here)

Comment on lines +51 to +65
if let err @ Err(_) = cmd!(sh, "cargo update -w -v --locked").run() {
// `Cargo.lock.msrv` is out of date! Probably from having bumped our own version number.
println! {"\
Error: `Cargo.lock.msrv` is out of date. \
Please run:\n \
(cp Cargo.lock{{.msrv,}} && cargo +{MSRV} update -w -v && cp Cargo.lock{{.msrv,}})\n\
\n\
Alternatively, `git apply` the `.patch` below:\
"}
cmd!(sh, "cargo update -q -w").quiet().run()?;
sh.copy_file("Cargo.lock", "Cargo.lock.msrv")?;
cmd!(sh, "git --no-pager diff --color=always -- Cargo.lock.msrv").quiet().run()?;
return err;
}
cmd!(sh, "cargo build --locked").run()?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Preview:

Screenshot 2023-05-28 at 22 07 17

@matklad
Copy link
Owner

matklad commented May 29, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented May 29, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 8f39b77 into matklad:master May 29, 2023
@danielhenrymantilla danielhenrymantilla deleted the force_mut-without-sync branch June 2, 2023 20:10
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 12, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [once_cell](https://github.com/matklad/once_cell) | dependencies | minor | `1.17.1` -> `1.18.0` |

---

### Release Notes

<details>
<summary>matklad/once_cell</summary>

### [`v1.18.0`](https://github.com/matklad/once_cell/blob/HEAD/CHANGELOG.md#&#8203;1180)

[Compare Source](matklad/once_cell@v1.17.2...v1.18.0)

-   `MSRV` is updated to 1.60.0 to take advantage of `dep:` syntax for cargo features,
    removing "implementation details" from publicly visible surface.

### [`v1.17.2`](https://github.com/matklad/once_cell/blob/HEAD/CHANGELOG.md#&#8203;1172)

[Compare Source](matklad/once_cell@v1.17.1...v1.17.2)

-   Avoid unnecessary synchronization in `Lazy::{force,deref}_mut()`, [#&#8203;231](matklad/once_cell#231).

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMTUuMiIsInVwZGF0ZWRJblZlciI6IjM1LjExNS4yIiwidGFyZ2V0QnJhbmNoIjoiZGV2ZWxvcCJ9-->

Co-authored-by: cabr2-bot <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1932
Reviewed-by: crapStone <[email protected]>
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nit: unnecessary synchronization in Lazy::force_mut()
2 participants