From 822deb28678597c59a08c7d223eb9bbe27648558 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 28 Aug 2018 20:43:45 +0300 Subject: [PATCH 1/3] Utils: do not block the cover_dl registry Accidently after f21398357bcc when a download would start, it would lock the cover_dl_registry hashmap till it had finished. Since the registry.read() happens on the main thread this would cause the UI to block until the download was and the mutex guard from the download thread dropped. --- podcasts-gtk/src/utils.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/podcasts-gtk/src/utils.rs b/podcasts-gtk/src/utils.rs index 29ef696..3a9553d 100644 --- a/podcasts-gtk/src/utils.rs +++ b/podcasts-gtk/src/utils.rs @@ -288,9 +288,14 @@ pub(crate) fn set_image_from_path( THREADPOOL.spawn(move || { if let Ok(mut guard) = COVER_DL_REGISTRY.write() { guard.insert(show_id); - if let Ok(pd) = dbqueries::get_podcast_cover_from_id(show_id) { - sender.send(downloader::cache_image(&pd)); - } + } + + // This operation is polling and will block the thread till the download is finished + if let Ok(pd) = dbqueries::get_podcast_cover_from_id(show_id) { + sender.send(downloader::cache_image(&pd)); + } + + if let Ok(mut guard) = COVER_DL_REGISTRY.write() { guard.remove(&show_id); } }); From 273c9f7b996d8616e54addd01434ac15364ad596 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 28 Aug 2018 20:53:56 +0300 Subject: [PATCH 2/3] Utils: Change the priority of the cover caches Since loadign a pixbuf from the pre-rendered cache is the most common operation and it does not affect the behavior we can first check that and then if the cover is midway downloading. This avoids a mutex lock for the most common path. --- podcasts-gtk/src/utils.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/podcasts-gtk/src/utils.rs b/podcasts-gtk/src/utils.rs index 3a9553d..2e69c87 100644 --- a/podcasts-gtk/src/utils.rs +++ b/podcasts-gtk/src/utils.rs @@ -251,21 +251,6 @@ pub(crate) fn set_image_from_path( show_id: i32, size: u32, ) -> Result<(), Error> { - // Check if there's an active download about this show cover. - // If there is, a callback will be set so this function will be called again. - // If the download succeeds, there should be a quick return from the pixbuf cache_image - // If it fails another download will be scheduled. - if let Ok(guard) = COVER_DL_REGISTRY.read() { - if guard.contains(&show_id) { - let callback = clone!(image => move || { - let _ = set_image_from_path(&image, show_id, size); - glib::Continue(false) - }); - gtk::timeout_add(250, callback); - return Ok(()); - } - } - if let Ok(hashmap) = CACHED_PIXBUFS.read() { // Check if the requested (cover + size) is already in the cache // and if so do an early return after that. @@ -284,6 +269,21 @@ pub(crate) fn set_image_from_path( } } + // Check if there's an active download about this show cover. + // If there is, a callback will be set so this function will be called again. + // If the download succeeds, there should be a quick return from the pixbuf cache_image + // If it fails another download will be scheduled. + if let Ok(guard) = COVER_DL_REGISTRY.read() { + if guard.contains(&show_id) { + let callback = clone!(image => move || { + let _ = set_image_from_path(&image, show_id, size); + glib::Continue(false) + }); + gtk::timeout_add(250, callback); + return Ok(()); + } + } + let (sender, receiver) = unbounded(); THREADPOOL.spawn(move || { if let Ok(mut guard) = COVER_DL_REGISTRY.write() { From 993b6e9d0a03d3ceceb6bf6bd79a21de94cca1d9 Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Tue, 28 Aug 2018 21:22:34 +0300 Subject: [PATCH 3/3] Utils: only queue a single cover download Before we were inserting the id of the cover into the registry from a rayon thread. But rayon will only execute N threads at the same time and let the rest into a queue. This would casue mutliple jobs being queued since the cover id was not inserted in the registry until the downloading had started. This fixes said behavior by having the main thread block and write in the id in the registry. --- podcasts-gtk/src/utils.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/podcasts-gtk/src/utils.rs b/podcasts-gtk/src/utils.rs index 2e69c87..82e484e 100644 --- a/podcasts-gtk/src/utils.rs +++ b/podcasts-gtk/src/utils.rs @@ -285,20 +285,22 @@ pub(crate) fn set_image_from_path( } let (sender, receiver) = unbounded(); - THREADPOOL.spawn(move || { - if let Ok(mut guard) = COVER_DL_REGISTRY.write() { - guard.insert(show_id); - } + if let Ok(mut guard) = COVER_DL_REGISTRY.write() { + // Add the id to the hashmap from the main thread to avoid queuing more than one downloads. + guard.insert(show_id); + drop(guard); - // This operation is polling and will block the thread till the download is finished - if let Ok(pd) = dbqueries::get_podcast_cover_from_id(show_id) { - sender.send(downloader::cache_image(&pd)); - } + THREADPOOL.spawn(move || { + // This operation is polling and will block the thread till the download is finished + if let Ok(pd) = dbqueries::get_podcast_cover_from_id(show_id) { + sender.send(downloader::cache_image(&pd)); + } - if let Ok(mut guard) = COVER_DL_REGISTRY.write() { - guard.remove(&show_id); - } - }); + if let Ok(mut guard) = COVER_DL_REGISTRY.write() { + guard.remove(&show_id); + } + }); + } let image = image.clone(); let s = size as i32;