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.
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
Use thread pool to decode image #31517
Conversation
🔨 Triggering try run (#8170481055) for Linux WPT |
Test results for linux-wpt-layout-2013 from try job (#8170481055): Flaky unexpected result (18)
Stable unexpected results that are known to be intermittent (13)
|
Test results for linux-wpt-layout-2020 from try job (#8170481055): Flaky unexpected result (14)
Stable unexpected results that are known to be intermittent (14)
|
✨ Try run (#8170481055) succeeded. |
Co-authored-by: Taym Haddadi <haddadi.taym@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, and the test results are unchanged. Ship it!
@@ -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), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Following the discussion on #31504, changes image decoding in
components/net/image_cache.rs
from creating a new thread into aCoreResourceThreadPool
(different from the one created incomponents/net/resource_thread.rs
).I would appreciate if someone could add the testing labels to check that automated tests keep working well, thanks!
./mach build -d
does not report any errors./mach test-tidy
does not report any errors