From 9098de22c16b2f48b5a01cd84365fe2906f3e4f1 Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Wed, 15 Apr 2020 17:27:46 +0000 Subject: [PATCH 1/5] Bump image from 0.23.2 to 0.23.3 Bumps [image](https://github.com/image-rs/image) from 0.23.2 to 0.23.3. - [Release notes](https://github.com/image-rs/image/releases) - [Changelog](https://github.com/image-rs/image/blob/master/CHANGES.md) - [Commits](https://github.com/image-rs/image/compare/v0.23.2...v0.23.3) Signed-off-by: dependabot-preview[bot] --- Cargo.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b590c228..a10187d5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -181,7 +181,7 @@ dependencies = [ [[package]] name = "image" -version = "0.23.2" +version = "0.23.3" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "bytemuck 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -311,7 +311,7 @@ dependencies = [ "clap 2.33.0 (registry+https://github.com/rust-lang/crates.io-index)", "cloudflare-zlib 0.2.5 (registry+https://github.com/rust-lang/crates.io-index)", "crc 1.8.1 (registry+https://github.com/rust-lang/crates.io-index)", - "image 0.23.2 (registry+https://github.com/rust-lang/crates.io-index)", + "image 0.23.3 (registry+https://github.com/rust-lang/crates.io-index)", "indexmap 1.3.2 (registry+https://github.com/rust-lang/crates.io-index)", "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)", @@ -478,7 +478,7 @@ dependencies = [ "checksum either 1.5.3 (registry+https://github.com/rust-lang/crates.io-index)" = "bb1f6b1ce1c140482ea30ddd3335fc0024ac7ee112895426e0a629a6c20adfe3" "checksum glob 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)" = "9b919933a397b79c37e33b77bb2aa3dc8eb6e165ad809e58ff75bc7db2e34574" "checksum hermit-abi 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)" = "1010591b26bbfe835e9faeabeb11866061cc7dcebffd56ad7d0942d0e61aefd8" -"checksum image 0.23.2 (registry+https://github.com/rust-lang/crates.io-index)" = "9062b90712d25bc6bb165d110aa59c6b47c849246e341e7b86a98daff9d49f60" +"checksum image 0.23.3 (registry+https://github.com/rust-lang/crates.io-index)" = "bfc5483f8d5afd3653b38a196c52294dcb239c3e1a5bade1990353ea13bcf387" "checksum indexmap 1.3.2 (registry+https://github.com/rust-lang/crates.io-index)" = "076f042c5b7b98f31d205f1249267e12a6518c1481e9dae9764af19b707d2292" "checksum inflate 0.4.5 (registry+https://github.com/rust-lang/crates.io-index)" = "1cdb29978cc5797bd8dcc8e5bf7de604891df2a8dc576973d71a281e916db2ff" "checksum itertools 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)" = "284f18f85651fe11e8a991b2adb42cb078325c996ed026d994719efcfca1d54b" From 11a7b68c4fde09acaadf8ab591da49e9e09bee5c Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Thu, 16 Apr 2020 10:39:10 +0000 Subject: [PATCH 2/5] Bump num_cpus from 1.12.0 to 1.13.0 Bumps [num_cpus](https://github.com/seanmonstar/num_cpus) from 1.12.0 to 1.13.0. - [Release notes](https://github.com/seanmonstar/num_cpus/releases) - [Changelog](https://github.com/seanmonstar/num_cpus/blob/master/CHANGELOG.md) - [Commits](https://github.com/seanmonstar/num_cpus/compare/v1.12.0...v1.13.0) Signed-off-by: dependabot-preview[bot] --- Cargo.lock | 8 ++++---- Cargo.toml | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a10187d5..3abdc079 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -295,7 +295,7 @@ dependencies = [ [[package]] name = "num_cpus" -version = "1.12.0" +version = "1.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "hermit-abi 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)", @@ -316,7 +316,7 @@ 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.12.0 (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)", @@ -353,7 +353,7 @@ dependencies = [ "crossbeam-queue 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "crossbeam-utils 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", - "num_cpus 1.12.0 (registry+https://github.com/rust-lang/crates.io-index)", + "num_cpus 1.13.0 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -492,7 +492,7 @@ dependencies = [ "checksum num-iter 0.1.40 (registry+https://github.com/rust-lang/crates.io-index)" = "dfb0800a0291891dd9f4fe7bd9c19384f98f7fbe0cd0f39a2c6b88b9868bbc00" "checksum num-rational 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)" = "da4dc79f9e6c81bef96148c8f6b8e72ad4541caa4a24373e900a36da07de03a3" "checksum num-traits 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)" = "c62be47e61d1842b9170f0fdeec8eba98e60e90e5446449a0545e5152acd7096" -"checksum num_cpus 1.12.0 (registry+https://github.com/rust-lang/crates.io-index)" = "46203554f085ff89c235cd12f7075f3233af9b11ed7c9e16dfe2560d03313ce6" +"checksum num_cpus 1.13.0 (registry+https://github.com/rust-lang/crates.io-index)" = "05499f3756671c15885fee9034446956fff3f243d6077b91e5767df161f766b3" "checksum png 0.16.1 (registry+https://github.com/rust-lang/crates.io-index)" = "46060468187c21c00ffa2a920690b29997d7fd543f5a4d400461e4a7d4fccde8" "checksum rayon 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)" = "db6ce3297f9c85e16621bb8cca38a06779ffc31bb8184e1be4bed2be4678a098" "checksum rayon-core 1.7.0 (registry+https://github.com/rust-lang/crates.io-index)" = "08a89b46efaf957e52b18062fb2f4660f8b8a4dde1807ca002690868ef2c85a9" diff --git a/Cargo.toml b/Cargo.toml index 55e409d9..c2b4f672 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ bit-vec = "^0.6.0" byteorder = "^1.0.0" crc = "^1.2.0" itertools = "^0.9.0" -num_cpus = "^1.0.0" +num_cpus = "^1.13.0" zopfli = "^0.4.0" miniz_oxide = "0.3" rgb = "0.8.11" From 9b8361171f1f92316d0ef680ecfeef91348548c7 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 16 Apr 2020 23:04:03 +0100 Subject: [PATCH 3/5] Revert "Unbreak no-default-features" This reverts commit 4ef92089d5743e896c920d6fa364ff66ab22c346. --- src/evaluate.rs | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/src/evaluate.rs b/src/evaluate.rs index f60a6e2f..ba58a8f0 100644 --- a/src/evaluate.rs +++ b/src/evaluate.rs @@ -17,6 +17,7 @@ use rayon; use rayon::prelude::*; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering::SeqCst; +#[cfg(feature = "parallel")] use std::sync::mpsc::*; use std::sync::Arc; use std::thread; @@ -46,30 +47,46 @@ pub(crate) struct Evaluator { nth: AtomicUsize, best_candidate_size: Arc, /// images are sent to the thread for evaluation + #[cfg(feature = "parallel")] eval_send: SyncSender, // the thread helps evaluate images asynchronously + #[cfg(feature = "parallel")] eval_thread: thread::JoinHandle>, + // in non-parallel mode, images are evaluated synchronously + #[cfg(not(feature = "parallel"))] + eval_best_candidate: Option, } impl Evaluator { pub fn new(deadline: Arc) -> Self { + #[cfg(feature = "parallel")] let (tx, rx) = sync_channel(4); Self { deadline, best_candidate_size: Arc::new(AtomicMin::new(None)), nth: AtomicUsize::new(0), + #[cfg(feature = "parallel")] eval_send: tx, + #[cfg(feature = "parallel")] eval_thread: thread::spawn(move || rx.into_iter().min_by_key(Candidate::cmp_key)), + #[cfg(not(feature = "parallel"))] + eval_best_candidate: None, } } /// Wait for all evaluations to finish and return smallest reduction /// Or `None` if all reductions were worse than baseline. + #[cfg(feature = "parallel")] fn get_best_candidate(self) -> Option { drop(self.eval_send); // disconnect the sender, breaking the loop in the thread self.eval_thread.join().expect("eval thread") } + #[cfg(not(feature = "parallel"))] + fn get_best_candidate(self) -> Option { + self.eval_best_candidate + } + pub fn get_result(self) -> Option { self.get_best_candidate().map(|candidate| candidate.image) } @@ -91,6 +108,7 @@ impl Evaluator { let best_candidate_size = self.best_candidate_size.clone(); // sends it off asynchronously for compression, // but results will be collected via the message queue + #[cfg(feature = "parallel")] let eval_send = self.eval_send.clone(); rayon::spawn(move || { let filters_iter = STD_FILTERS.par_iter().with_max_len(1); @@ -126,7 +144,18 @@ impl Evaluator { nth, }; - eval_send.send(new).expect("send"); + #[cfg(feature = "parallel")] + { + eval_send.send(new).expect("send"); + } + + #[cfg(not(feature = "parallel"))] + { + match self.eval_best_candidate { + Some(prev) if prev.cmp_key() < new.cmp_key() => {} + _ => self.eval_best_candidate = Some(new), + } + } } }); }); From 4fd9203800f88fa42363efea944951fd4d61ec19 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Thu, 16 Apr 2020 23:20:57 +0100 Subject: [PATCH 4/5] Bring back RefCell It was accidentally removed as part of #208. Fixes #214. --- src/evaluate.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/evaluate.rs b/src/evaluate.rs index ba58a8f0..254a0928 100644 --- a/src/evaluate.rs +++ b/src/evaluate.rs @@ -12,14 +12,15 @@ use crate::png::STD_WINDOW; #[cfg(not(feature = "parallel"))] use crate::rayon; use crate::Deadline; -#[cfg(feature = "parallel")] -use rayon; use rayon::prelude::*; +#[cfg(not(feature = "parallel"))] +use std::cell::RefCell; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering::SeqCst; #[cfg(feature = "parallel")] use std::sync::mpsc::*; use std::sync::Arc; +#[cfg(feature = "parallel")] use std::thread; struct Candidate { @@ -54,7 +55,7 @@ pub(crate) struct Evaluator { eval_thread: thread::JoinHandle>, // in non-parallel mode, images are evaluated synchronously #[cfg(not(feature = "parallel"))] - eval_best_candidate: Option, + eval_best_candidate: RefCell>, } impl Evaluator { @@ -70,7 +71,7 @@ impl Evaluator { #[cfg(feature = "parallel")] eval_thread: thread::spawn(move || rx.into_iter().min_by_key(Candidate::cmp_key)), #[cfg(not(feature = "parallel"))] - eval_best_candidate: None, + eval_best_candidate: RefCell::new(None), } } @@ -84,7 +85,7 @@ impl Evaluator { #[cfg(not(feature = "parallel"))] fn get_best_candidate(self) -> Option { - self.eval_best_candidate + self.eval_best_candidate.into_inner() } pub fn get_result(self) -> Option { @@ -151,9 +152,9 @@ impl Evaluator { #[cfg(not(feature = "parallel"))] { - match self.eval_best_candidate { + match &mut *self.eval_best_candidate.borrow_mut() { Some(prev) if prev.cmp_key() < new.cmp_key() => {} - _ => self.eval_best_candidate = Some(new), + best => *best = Some(new), } } } From bb332fa6d7981877b441ae7f8a644ba42f2f89ea Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Fri, 17 Apr 2020 09:49:40 +0100 Subject: [PATCH 5/5] 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))