[webkit-reviews] review denied: [Bug 181711] Limit the number of images that are asynchronously decoded at a time to half the number of cores : [Attachment 331892] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 181711: Limit the number of images that are asynchronously decoded at a
time to half the number of cores
https://bugs.webkit.org/show_bug.cgi?id=181711

Attachment 331892: Patch

https://bugs.webkit.org/attachment.cgi?id=331892&action=review




--- Comment #21 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 331892
  --> https://bugs.webkit.org/attachment.cgi?id=331892
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331892&action=review

> Source/WebCore/ChangeLog:12
> +	   We need to control the number of active threads which are created
for image
> +	   decoding. This has been an issue for power consumption and overall
system
> +	   degradation. This can be serious once the number of decoding thread
exceeds
> +	   the number of system available cores.

I don't think we have evidence that image decoding ever exceeds the number of
available cores. Do we? Can you supply that evidence?

It would be surprising if image decoding exceeded the number of available
cores, since WorkQueue uses libdispatch, whose primary job is to avoid creating
a number of threads that exceeds the number of available cores. Also, if it
were our goal to avoid using too many cores for image decoding, we would need
to coordinate that limitation across web processes and non-web CPU activity.
This patch doesn't do so, but again, luckily, libdispatch does.

Note that libdispatch offers a priority called "user initiated" which is lower
than "user interactive". If we want to guarantee that image decoding never
beats the main thread, we can test deprioritizing it to "user interactive".

I think I have to say r- because we can't accept an architectural change to
improve performance without a test case that demonstrates that performance
improved -- especially when we have reason to believe that the architecture
change duplicates existing functionality in a potentially worse way.