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

ndk-glue: Switch to parking_lot for mappable and Sendable lock guards #282

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented May 28, 2022

Depends on #282

It has come up previously that the native_window() function is easier to use if we could transpose a LockGuard<Option<>> into an Option<LockGuard<>>. After all, as soon as you have the lock you only need to check the Option<> value once and are thereafter guaranteed that its value won't change, until the lock is given up.

At the same time parking_lot has a send_guard feature which allows moving a lock guard to another thread (as long as deadlock_detection is disabled); a use case for this recently came up in glutin where the NativeWindow handle lock should be stored with a GL context so that our fn on_window_destroyed() callback doesn't return until after the Surface created on it is removed from the context and destroyed. Glutin forces this context to be Send, and a lock guard already allows Sync if NativeWindow is Sync (which it is).

@MarijnS95 MarijnS95 force-pushed the ndk-glue-parking-lot branch 2 times, most recently from 31e7175 to 130d013 Compare June 10, 2022 19:35
@MarijnS95 MarijnS95 marked this pull request as ready for review June 10, 2022 19:48
@MarijnS95 MarijnS95 added blocking a release Prevents a new release difficulty: hard Likely harder than most tasks here priority: high Vital to have type: enhancement Wouldn't this be the coolest? type: api Design and usability and removed type: enhancement Wouldn't this be the coolest? labels Jun 10, 2022
@MarijnS95
Copy link
Member Author

MarijnS95 commented Jun 11, 2022

I mentioned in #285 that we can constly construct the RwLocks from parking_lot without Lazy wrapper, but that would bump our MSRV to Rust 1.61 (current stable as of writing): Amanieu/parking_lot#325.

Is that something we want to do or have hard feelings against? We never cared much about MSRV and I wouldn't even know where this crate (the chain of these crates) are at right now.

If we do, we should at least test the MSRV that we set in the CI and specify it in Cargo.toml so that users get a sensible error, rather than a mid-crate compiler error stating that these functions are not const.


EDIT: In case it isn't clear, I don't think we should bump MSRV purely to drop a few Lazy<> wrappers around RwLocks - but it is interesting to consider if we have to maintain an MSRV at all, and apply this whenever we need 1.61 as MSRV for other reasons :)

@MarijnS95 MarijnS95 requested a review from msiglreith July 6, 2022 10:35
@MarijnS95 MarijnS95 force-pushed the ndk-glue-parking-lot branch from 130d013 to 22aabab Compare July 6, 2022 11:15
ndk-glue/src/lib.rs Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 force-pushed the ndk-glue-parking-lot branch from 22aabab to 4db6021 Compare July 8, 2022 12:01
…guards

It has come up previously that the `native_window()` function is easier
to use if we could transpose a `LockGuard<Option<>>` into an
`Option<LockGuard<>>`.  After all, as soon as you have the lock you only
need to check the `Option<>` value once and are thereafter guaranteed
that its value won't change, until the lock is given up.

At the same time `parking_lot` has a `send_guard` feature which allows
moving a lock guard to another thread (as long as `deadlock_detection`
is disabled); a use case for this recently came up [in glutin] where the
`NativeWindow` handle lock should be stored with a GL context so that
our `fn on_window_destroyed()` callback doesn't return until after the
`Surface` created on it is removed from the context and destroyed.
Glutin forces this context to be `Send`, and a lock guard already allows
`Sync` if `NativeWindow` is `Sync` (which it is).

[in glutin]: rust-windowing/glutin#1411 (comment)
@MarijnS95 MarijnS95 force-pushed the ndk-glue-parking-lot branch from 4db6021 to 286674b Compare July 8, 2022 12:03
@MarijnS95 MarijnS95 merged commit a0c5e99 into rust-mobile:master Jul 8, 2022
@MarijnS95 MarijnS95 deleted the ndk-glue-parking-lot branch July 8, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking a release Prevents a new release difficulty: hard Likely harder than most tasks here priority: high Vital to have type: api Design and usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants