Make `parking_lot` dependency optional

[fschutt]

Right now winit depends on parking_lot for Mutex handling - while I realize that parking_lot offers faster Mutex implementations than the standard library, it is also a pretty heavy dependency:

parking_lot v0.6.3
├── lock_api v0.1.3
│   ├── owning_ref v0.3.3
│   │   └── stable_deref_trait v1.1.1
│   └── scopeguard v0.3.3
└── parking_lot_core v0.2.14
    ├── rand v0.4.3
    └── smallvec v0.6.5
        └── unreachable v1.0.0
            └── void v1.0.2

This adds a lot of overhead to the compile time. winit is currently using parking_lot pretty much only for the Mutex, which can be exchanged with the std::sync::Mutex, so a simple feature flag should be possible so that crates depending on winit can turn off the parking_lot mutex and use the standard library mutex - many crates don't need that extra performance and would rather go with better compile times.

[Osspial]

I don't think this is really necessary. The extra compile time parking_lot adds is a cost that's only going to get paid on the initial build (which on my machine was a 2-3 second difference, although that was with everything already downloaded). After that, the performance gains are effectively free. I'd be willing to bet that in the vast majority of cases, the amount of time necessary to discover that a feature flag for this exists, consider whether or not to enable it, then finally enable/disable it will dwarf the amount of time it actually takes to compile.

[tomaka]

The compilation time of a crate is not proportional to the number of its dependencies.
In particular, I don't think parking_lot is slow to compile.

[fschutt]

I know about the theory that it shouldn't be slower, but that's not reality, sorry. The more dependencies, the slower the build time.

I know it's "only" 3 seconds, but I have projects with more than 300 dependencies and each dependency takes "only" 3 seconds to compile. I have compile times of 10 - 20 minutes, if I can shave off 3 seconds - it's still something! And no, even after compilation the cost of these dependencies isn't free - cargos caching is far, far from perfect. Every time I switch between cargo check and cargo build it takes "only" 3 seconds more. Every time I switch between debug and release or from stable to nightly and back, "only" 3 seconds. When I build with LTO, it takes 15 seconds more because LTO is dog slow the more libraries you have. Every time I make a new build on travis or appveyor it takes "only" 10 seconds more because travis isn't very fast either. On every single commit. Every time I do cargo doc and cargo is too stupid to figure out that some dependencies are already checked and built it takes "only" 3 seconds more.

And you forgot link time, every dependency adds to the time it takes to link all libraries together into one executable, which is especially slow because it's mostly single-threaded. Take your 3 seconds, multiply them by 300 and maybe you get why I'd like to cut down on dependencies. It's not like my entire application consists of just winit.

I'm already trying to optimize all my other crates and cut down on unnecessary dependencies, but this is simply an easy opportunity to cut down on unnecessary dependencies. Right now I am looking through the Cargo.toml files of all projects I depend on looking for feature flags to disable. And every single dependency I can shave off is good and this would be a pretty easy opportunity.

But alright - if you don't think it's necessary, close my PR and I'll just maintain a fork.

[anderejd]

If this is easy to implement and doesn't add much extra code to maintain, then this request makes sense to me. 👍

[tomaka]

doesn't add much extra code to maintain

That's where I disagree. This makes the code more complex for almost nothing.

[Osspial]

@fschutt If you've got over 300 dependencies, I'd be mildly surprised if parking_lot wasn't getting pulled in somewhere else as well! Regardless, if you're looking to cut down on build times, it's probably worth trying to find larger culprits than parking_lot - I'd guess that there are more substantial crates that are weighing down on your build times much more heavily, and if you can cut those out you don't have to go through the extensive process of petitioning a bunch of crate owners to make small performance-improving crates optional.

Also, I know this is addressing a fairly small part of your comment, but if you want to build docs for only your top-level crate you can do cargo doc --no-deps. That'll be the single biggest improvement to documentation performance you can make without going way deep into the weeds.

[anderejd]

As a mere user of winit who don't know the internals well enough I can't really have a strong opinion on this. So I don't.

I got curious though and took a look at the related PR and I think it can be rewritten in much smaller and fewer changes. Winit could use a winit::sync::Mutex which could be a simple type alias in the case of the parking_lot feature being enabled and in the case of the parking_lot feature being disabled it can be a wrapper type for the std::sync::Mutex that injects mutex.lock().unwrap() in the lock method. That way the only change needed throughout winit is to use the winit Mutex.

[fschutt]

@anderejd That is certainly a better option - my PR was more about proving that this "easy exchange" of std::sync::Mutex and parking_lot::Mutex was possible (because the only function used on the mutex is the .lock() function, which is available on both types). If written correctly, I'd guess that it would be possible in 50 lines max, at which point the "more complex code" argument would fall flat.

I do get the argument that it's harder to read (which, as above, can be fixed), but I don't get the argument that winit can be sloppy with its dependencies just because other packages are sloppy too. I've already stripped out duplicates and no, parking_lot doesn't appear twice - for example there are lots of duplicates of euclid, but I'm not going to tell other repositories to exchange that, because euclid isn't easy to exchange and it actually does something useful. But here, changing parking_lot::Mutex to std::sync::Mutex is a relatively easy change, otherwise I wouldn't bother writing a PR. There are dependencies which are necessary (ex. wayland bindings or euclid - you can't just "replace" or strip them, they actually do some useful work) and then there are fringe dependencies, like crates for syntax sugar or in this case, performance optimization, which can be turned off without large code changes.

So I think I'll see how much code it is to implement @anderejd s solution, so that less code changes are necessary and do a second PR and then we'll see if the idea is more feasible.

[elinorbgr]

For the record, the parking_lot dependency was added in #491 with the following reason:

Switched from std::sync::Mutex to parking_lot::Mutex (within [the x11 backend]). There appears to be no downside to this, but if anyone finds one, this would be easy to revert.

Regarding this, I agree that parking_lot appears to be a pretty minor dependency, but I think we could also wonder about the gains it provides. My understanding is that the main selling point of parking_lot over std's Mutex is performance. Does winit have a such a Mutex-heavy workflow that this performance difference matters? Or are there other ways in which parking_lot is better than std?

In general, I'd tend to believe having few dependencies is a nice thing to have:

• as raised in this issue, it makes build times smaller
• winit wants to maintain compatibility with older versions of rustc, which can be tricky as dependencies upgrade their requirements (I personally had this issue just yesterday with wayland-client)
  • however here it appears parking_lot handles this cleanly, making bumping their minimal rustc version as a breaking change
• having a lot of dependencies means having many crates to track for updates, to avoid depending on old, outdated version with flaws and/or security risks

Regarding that, I agree that having parking_lot as an optional dependency is a bad option: it removes nothing of the maintenance costs, and actually increases them.

Thus I believe the question is rather does winit actually need parking_lot as a dependency? If yes, why?

[bjorn-ove]

I was looking into compile time today and was surprised to find that winit was one of my biggest culprits. This surprised me because the crate appears to be a lightweight windowing only library. I guess this is the price to pay if you need to support multiple platforms.

While searching to see if performance was actively being worked on I stumbled upon this issue. For me cold compile times are really important, and the argument that an incremental build is faster does not have any weight.

What stood out to me however was the quote from #491 above.

Switched from std::sync::Mutex to parking_lot::Mutex (within this backend). There appears to be no downside to this, but if anyone finds one, this would be easy to revert.

This seems to be a flawed design philosophy to me. Moving away from the standard library should always be backed by a well founded reason. The fact that something uses less instructions and therefore is faster should not be enough to introduce a change like this. It should be backed by benchmarks proving it is faster, and those benchmarks needs to be real-world, not micro-benchmarks.

In most cases, which mutex type used does not affect performance in any way because it is such a small part of the overall time spent. If it does affect performance, it is generally a sign that you are doing it wrong. Very often a mutex is the wrong choice in the first place if the contention is that high.

With that said, most of the slow compile in my case came from wayland and after disabling that I gained 24 seconds (debug) which is great.

Keep up the good work, I know how hard window handling can be

[fschutt]

It's ironic because std::mutex::Mutex will use parking_lot::Mutex in the future: rust-lang/rust#56410. Currently this is blocked on faern/parking_lot#1, but at some point in the future, std will switch to parking_lot. I think when that's done, parking_lot can be safely removed.

It's correct that parking_lot is faster (rust-lang/rust#56410 (comment)), but winit doesn't use mutexes in the critical paths, only during startup and to manage resources correctly. I already did the PR to remove parking_lot, so it's just a style decision.

For build times, I now use a DLL, I compile every crate I need into a DLL and then dynamically load it. Makes recompilation + linking much faster.

[bjorn-ove]

It's ironic because std::mutex::Mutex will use parking_lot::Mutex in the future

My comment is more about the principle, I did not do my research regarding this particular case. I have seen several PR's within the last year where projects change away from std to some other faster version on a hunch that it should be faster. I just wanted to make the point that the question should be "does it make things better", instead of "does it make things worse".

For build times, I now use a DLL, I compile every crate I need into a DLL and then dynamically load it. Makes recompilation + linking much faster.

I have also noticed many people doing this and other hacks lately, and worry that this is a sign of sickness in the language. I worry things are taking a turn for the worse with clever people working around the problem instead of fixing the root cause which is the unnecessarily slow compilation.

Anyway, I guess this is more of a philosophical question that I should have started on Rust Internals instead.