From bb332fa6d7981877b441ae7f8a644ba42f2f89ea Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Fri, 17 Apr 2020 09:49:40 +0100 Subject: [PATCH] Don't override global rayon pool (#211) Rayon uses a singleton global pool. By default it's set to a regular spawn handler with number of logical CPU cores, but it can be overridden by Rust applications to customize number of threads, spawn handlers, exit handlers and other options. Such customization should be usually done at the app level, because if a single library initialises the global pool, then Rayon will prevent any further overrides and they will error out. This can cause conflicts between libraries or library and user code and make them impossible to use together. Hence, I've removed the `threads` option from the `Options` struct and instead moved initialisation to the CLI part of the codebase (main.rs). Users of the library that didn't depend on custom `threads` number can keep using it as before - they'll still get same number of threads as number of logical CPU cores, while users who need fine-tuning, can do that by customizing rayon pool themselves at the top level of the app. Note: another alternative to keep the option could've been to use `ThreadPoolBuilder::build` + `ThreadPool::install` to use a local pool just within OxiPNG, but that would ignore any customizations made by users in top-level pool and would prevent usage on targets that require custom spawn handlers like WebAssembly. As such, I've decided to avoid it. Co-authored-by: Josh Holmer --- Cargo.lock | 1 - Cargo.toml | 1 - src/lib.rs | 18 ------------------ src/main.rs | 7 ++++++- 4 files changed, 6 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3abdc079..e7a5debc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -316,7 +316,6 @@ dependencies = [ "itertools 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", "libdeflater 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "miniz_oxide 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", - "num_cpus 1.13.0 (registry+https://github.com/rust-lang/crates.io-index)", "rayon 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "rgb 0.8.16 (registry+https://github.com/rust-lang/crates.io-index)", "wild 2.0.2 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/Cargo.toml b/Cargo.toml index c2b4f672..bcbfa0eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,6 @@ bit-vec = "^0.6.0" byteorder = "^1.0.0" crc = "^1.2.0" itertools = "^0.9.0" -num_cpus = "^1.13.0" zopfli = "^0.4.0" miniz_oxide = "0.3" rgb = "0.8.11" diff --git a/src/lib.rs b/src/lib.rs index 3395510b..9b498728 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,7 +13,6 @@ #![warn(clippy::range_plus_one)] #![allow(clippy::cognitive_complexity)] -use num_cpus; #[cfg(feature = "parallel")] extern crate rayon; #[cfg(not(feature = "parallel"))] @@ -214,10 +213,6 @@ pub struct Options { /// /// Default: `false` pub use_heuristics: bool, - /// Number of threads to use - /// - /// Default: number of CPU cores - pub threads: usize, /// Maximum amount of time to spend on optimizations. /// Further potential optimizations are skipped if the timeout is exceeded. @@ -323,7 +318,6 @@ impl Default for Options { strip: Headers::None, deflate: Deflaters::Zlib, use_heuristics: false, - threads: num_cpus::get(), timeout: None, } } @@ -331,12 +325,6 @@ impl Default for Options { /// Perform optimization on the input file using the options provided pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<()> { - // Initialize the thread pool with correct number of threads - #[cfg(feature = "parallel")] - let _ = rayon::ThreadPoolBuilder::new() - .num_threads(opts.threads) - .build_global(); - // Read in the file and try to decode as PNG. if opts.verbosity.is_some() { eprintln!("Processing: {}", input); @@ -431,12 +419,6 @@ pub fn optimize(input: &InFile, output: &OutFile, opts: &Options) -> PngResult<( /// Perform optimization on the input file using the options provided, where the file is already /// loaded in-memory pub fn optimize_from_memory(data: &[u8], opts: &Options) -> PngResult> { - // Initialize the thread pool with correct number of threads - #[cfg(feature = "parallel")] - let _ = rayon::ThreadPoolBuilder::new() - .num_threads(opts.threads) - .build_global(); - // Read in the file and try to decode as PNG. if opts.verbosity.is_some() { eprintln!("Processing from memory"); diff --git a/src/main.rs b/src/main.rs index bafdc2bd..8f03e79c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -490,7 +490,12 @@ fn parse_opts_into_struct( } if let Some(x) = matches.value_of("threads") { - opts.threads = x.parse::().unwrap(); + let threads = x.parse::().unwrap(); + + rayon::ThreadPoolBuilder::new() + .num_threads(threads) + .build_global() + .map_err(|err| err.to_string())?; } Ok((out_file, out_dir, opts))