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.

Sign up for GitHub

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

Jump to bottom

Use thread pool to decode image #31517

Merged
merged 2 commits into from Mar 7, 2024
Merged

Use thread pool to decode image #31517

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b69e07e
feat: do not spawn dedicated thread to decode image
eerii Mar 5, 2024
a883dc4
fix: change comment to docstring
eerii Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions components/net/image_cache.rs
View file
Open in desktop
Expand Up @@ -4,8 +4,8 @@

use std::collections::hash_map::Entry::{Occupied, Vacant};
use std::collections::HashMap;
use std::mem;
use std::sync::{Arc, Mutex};
use std::{mem, thread};

use embedder_traits::resources::{self, Resource};
use imsz::imsz_from_reader;
Expand All @@ -25,6 +25,8 @@ use servo_url::{ImmutableOrigin, ServoUrl};
use webrender_api::units::DeviceIntSize;
use webrender_api::{ImageData, ImageDescriptor, ImageDescriptorFlags, ImageFormat};

use crate::resource_thread::CoreResourceThreadPool;

///
/// TODO(gw): Remaining work on image cache:
/// * Make use of the prefetch support in various parts of the code.
Expand Down Expand Up @@ -415,6 +417,9 @@ impl ImageCacheStore {

pub struct ImageCacheImpl {
store: Arc<Mutex<ImageCacheStore>>,

/// Thread pool for image decoding
thread_pool: CoreResourceThreadPool,
}

impl ImageCache for ImageCacheImpl {
Expand All @@ -431,6 +436,7 @@ impl ImageCache for ImageCacheImpl {
placeholder_url: ServoUrl::parse("chrome://resources/rippy.png").unwrap(),
webrender_api: webrender_api,
})),
thread_pool: CoreResourceThreadPool::new(16),
Copy link
Member

Choose a reason for hiding this comment

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

Why was 16 chosen here? This seems like too many threads for CPUs with fewer cores. Perhaps this can be limited to the number of cpus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It put it like that since it was the same as in components/net/resource_thread.rs, but here it probably makes more sense to limit it since it is a secondary thread pool. Should I set it to the number of cores then?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, perhaps the number of CPUs is a better number. Resource loading is network bound, while decoding is CPU bound.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#31585 should address this changes

}
}

Expand Down Expand Up @@ -634,7 +640,7 @@ impl ImageCache for ImageCacheImpl {
};

let local_store = self.store.clone();
thread::spawn(move || {
self.thread_pool.spawn(move || {
let msg = decode_bytes_sync(key, &*bytes, cors_status);
debug!("Image decoded");
local_store.lock().unwrap().handle_decoder(msg);
Expand Down