Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1143473002:
Re-land: content: Use NumberOfProcessors() / 2 raster threads. (Closed)
|
Created:
5 years, 7 months ago by reveman Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, piman+watch_chromium.org, aelias_OOO_until_Jul13 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-land: content: Use NumberOfProcessors() / 2 raster threads.
4 raster threads improve performance on 8 core machines.
8 core machines might not be a priority but it seems
like an unnecessary limitation to have.
BUG=
Committed: https://crrev.com/fb8bb92ed3f577986d513789ced910417dc04326
Cr-Commit-Position: refs/heads/master@{#330555}
Patch Set 1 #
Total comments: 4
Patch Set 2 : cleanup and reduce kMaxRasterThreads to 16 #
Total comments: 2
Patch Set 3 : fix async_uploads_is_used #
Total comments: 1
Messages
Total messages: 34 (9 generated)
reveman@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org
https://codereview.chromium.org/1143473002/diff/1/content/browser/gpu/composi... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/1143473002/diff/1/content/browser/gpu/composi... content/browser/gpu/compositor_util.cc:36: const int kMaxRasterThreads = 64; does 64 threads really ever make sense? at what point does a tile per thread get ridiculous? https://codereview.chromium.org/1143473002/diff/1/content/browser/gpu/composi... content/browser/gpu/compositor_util.cc:216: kMinRasterThreads, kMaxRasterThreads); nit: how about do the computation for a number here and below without clamping, then clamp at the end once.
Would it make sense to clamp this at like 4? At what point should the compositor not be eating up more raster threads?
https://codereview.chromium.org/1143473002/diff/1/content/browser/gpu/composi... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/1143473002/diff/1/content/browser/gpu/composi... content/browser/gpu/compositor_util.cc:36: const int kMaxRasterThreads = 64; On 2015/05/13 20:01:23, danakj wrote: > does 64 threads really ever make sense? at what point does a tile per thread get > ridiculous? We're going to use these threads for more than raster in the near future (e.g. video plane copy operations, fav icon decode, etc.) so it's better to think of these as child process worker threads. Yes, the command line switch and these constants needs to be renamed at some point. I'm happy to lower it though, just don't know to what. https://codereview.chromium.org/1143473002/diff/1/content/browser/gpu/composi... content/browser/gpu/compositor_util.cc:216: kMinRasterThreads, kMaxRasterThreads); On 2015/05/13 20:01:23, danakj wrote: > nit: how about do the computation for a number here and below without clamping, > then clamp at the end once. I did that first but didn't like it as it's already clamped when force_num_raster_threads is true. I guess I can refactor ForceNumberOfRendererRasterThreads a bit to make more sense.
reduced max worker threads to 16 but I'm happy to reduce it more if you like. ptal.
Ok, lgtm. If 16 is too many in the future, we can reduce it then.
LGTM
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1143473002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
reveman@chromium.org changed reviewers: + piman@chromium.org
+piman for owner review
ping for owner review
lgtm
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1143473002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4a0177a1dba8197f61021ff3b4f9426e45faaa62 Cr-Commit-Position: refs/heads/master@{#330427}
Message was sent while issue was closed.
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1143473002/diff/20001/content/browser/gpu/com... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/1143473002/diff/20001/content/browser/gpu/com... content/browser/gpu/compositor_util.cc:220: IsZeroCopyUploadEnabled() || IsOneCopyUploadEnabled(); Is this condition supposed to be negated?
Message was sent while issue was closed.
https://codereview.chromium.org/1143473002/diff/20001/content/browser/gpu/com... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/1143473002/diff/20001/content/browser/gpu/com... content/browser/gpu/compositor_util.cc:220: IsZeroCopyUploadEnabled() || IsOneCopyUploadEnabled(); On 2015/05/19 16:37:04, vmpstr wrote: > Is this condition supposed to be negated? Oops. I'll revert and re-land a fixed version of this patch. Thanks!
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1136633006/ by reveman@chromium.org. The reason for reverting is: Incorrect logic for determining if async uploads is used.
lgtm, thanks!
The CQ bit was checked by reveman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, enne@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1143473002/#ps40001 (title: "fix async_uploads_is_used")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1143473002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fb8bb92ed3f577986d513789ced910417dc04326 Cr-Commit-Position: refs/heads/master@{#330555}
Message was sent while issue was closed.
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1143473002/diff/40001/content/browser/gpu/com... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/1143473002/diff/40001/content/browser/gpu/com... content/browser/gpu/compositor_util.cc:214: int num_raster_threads = base::SysInfo::NumberOfProcessors() / 2; What does this return on octacore big/little ARM devices? We really want it to be "4", but I fear this returns "8". Android has a special API to get the CPU count, and I believe it returns 4 for such devices, but it looks like we're using the posix "sysconf(_SC_NPROCESSORS_CONF)"... wonder if we should switch the Android implementation for NumberOfProcessors() to use android_getCpuCount() from the NDK.
Message was sent while issue was closed.
On 2015/05/28 at 23:23:31, jdduke wrote: > https://codereview.chromium.org/1143473002/diff/40001/content/browser/gpu/com... > File content/browser/gpu/compositor_util.cc (right): > > https://codereview.chromium.org/1143473002/diff/40001/content/browser/gpu/com... > content/browser/gpu/compositor_util.cc:214: int num_raster_threads = base::SysInfo::NumberOfProcessors() / 2; > What does this return on octacore big/little ARM devices? We really want it to be "4", but I fear this returns "8". > > Android has a special API to get the CPU count, and I believe it returns 4 for such devices, but it looks like we're using the posix "sysconf(_SC_NPROCESSORS_CONF)"... wonder if we should switch the Android implementation for NumberOfProcessors() to use android_getCpuCount() from the NDK. Using android_getCpuCount() to implement NumberOfProcessors() on Android sgtm if that's the preferred way to get the cpu count.
Message was sent while issue was closed.
On 2015/05/29 00:23:08, reveman wrote: > On 2015/05/28 at 23:23:31, jdduke wrote: > > > https://codereview.chromium.org/1143473002/diff/40001/content/browser/gpu/com... > > File content/browser/gpu/compositor_util.cc (right): > > > > > https://codereview.chromium.org/1143473002/diff/40001/content/browser/gpu/com... > > content/browser/gpu/compositor_util.cc:214: int num_raster_threads = > base::SysInfo::NumberOfProcessors() / 2; > > What does this return on octacore big/little ARM devices? We really want it to > be "4", but I fear this returns "8". > > > > Android has a special API to get the CPU count, and I believe it returns 4 for > such devices, but it looks like we're using the posix > "sysconf(_SC_NPROCESSORS_CONF)"... wonder if we should switch the Android > implementation for NumberOfProcessors() to use android_getCpuCount() from the > NDK. > > Using android_getCpuCount() to implement NumberOfProcessors() on Android sgtm if > that's the preferred way to get the cpu count. android_getCpuCount() also returns 8 on the S6 unfortunately. We may need to special case the formula for Android. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
