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

Remove parking_lot dependency #2423

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Aug 11, 2022

Fixes #650.

std's Mutex has recently improved a lot, so there's not really a performance argument for keeping i any more. Also means we won't have to use the same windows-sys version as parking_lot

Dependency tree (Windows):

// Before
winit v0.27.1
├── bitflags v1.3.2
├── instant v0.1.12
│   └── cfg-if v1.0.0
├── log v0.4.17
│   └── cfg-if v1.0.0
├── once_cell v1.13.0
├── parking_lot v0.12.1
│   ├── lock_api v0.4.7
│   │   └── scopeguard v1.1.0
│   │   [build-dependencies]
│   │   └── autocfg v1.1.0
│   └── parking_lot_core v0.9.3
│       ├── cfg-if v1.0.0
│       ├── smallvec v1.9.0
│       └── windows-sys v0.36.1
│           └── windows_x86_64_gnu v0.36.1
├── raw-window-handle v0.5.0
│   └── cty v0.2.2
└── windows-sys v0.36.1 (*)

// After
winit v0.27.1
├── bitflags v1.3.2
├── instant v0.1.12
│   └── cfg-if v1.0.0
├── log v0.4.17
│   └── cfg-if v1.0.0
├── once_cell v1.13.0
├── raw-window-handle v0.5.0
│   └── cty v0.2.2
└── windows-sys v0.36.1
    └── windows_x86_64_gnu v0.36.1

Dependency tree (X11):

// Before
winit v0.27.1
├── bitflags v1.3.2
├── instant v0.1.12
│   └── cfg-if v1.0.0
├── libc v0.2.129
├── log v0.4.17
│   └── cfg-if v1.0.0
├── mio v0.8.4
│   ├── libc v0.2.129
│   └── log v0.4.17 (*)
├── once_cell v1.13.0
├── parking_lot v0.12.1
│   ├── lock_api v0.4.7
│   │   └── scopeguard v1.1.0
│   │   [build-dependencies]
│   │   └── autocfg v1.1.0
│   └── parking_lot_core v0.9.3
│       ├── cfg-if v1.0.0
│       ├── libc v0.2.129
│       └── smallvec v1.9.0
├── percent-encoding v2.1.0
├── raw-window-handle v0.5.0
│   └── cty v0.2.2
└── x11-dl v2.20.0
    ├── lazy_static v1.4.0
    └── libc v0.2.129
    [build-dependencies]
    └── pkg-config v0.3.25

// After
winit v0.27.1
├── bitflags v1.3.2
├── instant v0.1.12
│   └── cfg-if v1.0.0
├── libc v0.2.129
├── log v0.4.17
│   └── cfg-if v1.0.0
├── mio v0.8.4
│   ├── libc v0.2.129
│   └── log v0.4.17 (*)
├── once_cell v1.13.0
├── percent-encoding v2.1.0
├── raw-window-handle v0.5.0
│   └── cty v0.2.2
└── x11-dl v2.20.0
    ├── lazy_static v1.4.0
    └── libc v0.2.129
    [build-dependencies]
    └── pkg-config v0.3.25
  • Tested on all platforms changed
    • Nope, please test this!
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

This looks good to me; I'll always support removing a dependency that's been obsoleted by the standard library.

@@ -56,7 +56,7 @@ pub type XlibErrorHook =
pub fn register_xlib_error_hook(hook: XlibErrorHook) {
// Append new hook.
unsafe {
XLIB_ERROR_HOOKS.lock().push(hook);
XLIB_ERROR_HOOKS.lock().unwrap().push(hook);
Copy link
Member

Choose a reason for hiding this comment

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

While this is entirely fine, I'd suggest having a macro like this somewhere in the crate:

macro_rules! lock {
    ($mutex: expr) => {{
        match ($mutex).lock() {
            Ok(guard) => guard,
            Err(err) => {
                log::warn!("Mutex is poisoned: {:?}", &err);
                err.into_inner()
            }
        }
    }}
}

And instead calling it like this:

lock!(XLIB_ERROR_HOOKS).push(hook);

This ensures two things:

  1. A poisoned lock won't blow up the entire system.
  2. If we decide to switch to another mutex scheme, for some reason, we can easily adjust lock! to fit our use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with unwrap because it's what's used in the other backends, and I'm not at all sure how to handle mutex poisoning

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess I should add it in a separate PR then...

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be nice, please do!

Copy link
Member

Choose a reason for hiding this comment

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

Great! I'll wait until after this PR is merged, so I can make a macro that works across the entire crate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @notgull, this PR has been merged, so if you want to make this change, now you have the opportunity!

@madsmtm madsmtm force-pushed the remove-parking-lot branch 3 times, most recently from 5dd1bd2 to 9095ed4 Compare August 12, 2022 12:52
@madsmtm madsmtm force-pushed the remove-parking-lot branch 2 times, most recently from ae8b967 to 19ba050 Compare August 31, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Make parking_lot dependency optional
3 participants