Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1830693004:
content: Depict priority in raster worker thread's name. (Closed)
|
Created:
4 years, 9 months ago by prashant.n Modified:
4 years, 8 months ago CC:
blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, kinuko+watch, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncontent: Depict priority in raster worker thread's name.
Adding priority in raster worker thread's name distinguishes different
tile worker threads in traces, which will help analyse the raster tasks
run by those threads to further level.
The normal priority thread is prefixed with "CompositorTileWorker",
while background priority thread is prefixed with
"CompositorTileWorkerBackground".
BUG=None
Committed: https://crrev.com/af5e7fd4d6d8a97444ef0e605491b24be08d5adf
Cr-Commit-Position: refs/heads/master@{#383703}
Patch Set 1 #
Total comments: 2
Patch Set 2 : simplify. #
Total comments: 1
Patch Set 3 : review comments. #Messages
Total messages: 31 (14 generated)
prashant.n@samsung.com changed reviewers: + ericrk@chromium.org, jam@chromium.org, reveman@chromium.org, vmpstr@chromium.org
ptal.
https://codereview.chromium.org/1830693004/diff/1/content/renderer/raster_wor... File content/renderer/raster_worker_pool.cc (left): https://codereview.chromium.org/1830693004/diff/1/content/renderer/raster_wor... content/renderer/raster_worker_pool.cc:158: base::StringPrintf("CompositorTileWorker%u", Can we reduce this patch to a one line change here at l.158 that simply s/CompositorTileWorker/CompositorBackgroundTileWorker/ ?
https://codereview.chromium.org/1830693004/diff/1/content/renderer/raster_wor... File content/renderer/raster_worker_pool.cc (left): https://codereview.chromium.org/1830693004/diff/1/content/renderer/raster_wor... content/renderer/raster_worker_pool.cc:158: base::StringPrintf("CompositorTileWorker%u", On 2016/03/24 15:40:46, reveman wrote: > Can we reduce this patch to a one line change here at l.158 that simply > s/CompositorTileWorker/CompositorBackgroundTileWorker/ ? Okay. I'll reduce the patch. May be later we can add if we have many priorities for different task workers.
Description was changed from
==========
content: Add priority in raster worker thread's name.
Adding priority (abbreviated) in raster worker thread's name
distinguishes different tile worker threads in traces, which will help
analyse the raster tasks run by those threads to further level.
This patch also adds abbreviations for the priorities as given below.
BACKGROUND bg
NORMAL nl
DISPLAY ds
REALTIME_AUDIO rt
BUG=None
==========
to
==========
content: Add priority in raster worker thread's name.
Adding priority (abbreviated) in raster worker thread's name
distinguishes different tile worker threads in traces, which will help
analyse the raster tasks run by those threads to further level.
The nomral priority thread is prefixed with CompositorTileWorker, whereas
background priority thread is prefixed with
CompositorBackgroundTileWorker. However thread number in prefix is kept
increasing only.
BUG=None
==========
Description was changed from ========== content: Add priority in raster worker thread's name. Adding priority (abbreviated) in raster worker thread's name distinguishes different tile worker threads in traces, which will help analyse the raster tasks run by those threads to further level. The nomral priority thread is prefixed with CompositorTileWorker, whereas background priority thread is prefixed with CompositorBackgroundTileWorker. However thread number in prefix is kept increasing only. BUG=None ========== to ========== content: Depict priority in raster worker thread's name. Adding priority in raster worker thread's name distinguishes different tile worker threads in traces, which will help analyse the raster tasks run by those threads to further level. The normal priority thread is prefixed with "CompositorTileWorker", while background priority thread is prefixed with "CompositorBackgroundTileWorker". However thread number in prefix is kept increasing only. BUG=None ==========
ptal.
lgtm fyi this will likely affect some of our perf metrics: https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/metrics...
On 2016/03/24 17:52:51, prashant.n wrote: > ptal. Looks like there are few dependencies for the thread name. e.g. src/tools/perf/metrics/timeline.py. When searched in codebase, there are many places where prefix "CompositorTileWorker" is used. How about changing prefix to "CompositorTileWorker" such that CompositorTileWorkerForeground, CompositorTileWorkerBackground Or how about changing the prefix itself to "TaskWorker" as threads work on tasks than tiles. I was thinking adding original change, but priority as suffix after tid. This would be applicable to all the threads. But there could be incosistency if thread's priority gets changed. In such case we either need to keep original priority as suffix OR change according to current priority. e.g. CompositorTileWorker1/14567(nl), CompositorTileWorker2/14569(bg). Pls. let me know your opinion.
On 2016/03/25 05:44:10, reveman wrote: > lgtm > > fyi this will likely affect some of our perf metrics: > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/metrics... Hmm, I also found the same problem :).
One query, not related to this patch - why raster_worker_pool is in content/renderer and not in cc/raster?
On 2016/03/25 at 06:19:15, prashant.n wrote: > One query, not related to this patch - why raster_worker_pool is in content/renderer and not in cc/raster? This is used for more than raster these days. It's used by media/ for video decode. These threads and the class itself is not named properly anymore. We should probably rename it to something like "PrioritizedWorkerPool" or "CategorizedWorkerPool" with matching thread name changes. https://codereview.chromium.org/1830693004/diff/20001/content/renderer/raster... File content/renderer/raster_worker_pool.cc (right): https://codereview.chromium.org/1830693004/diff/20001/content/renderer/raster... content/renderer/raster_worker_pool.cc:158: base::StringPrintf("CompositorBackgroundTileWorker%u", Maybe replace the "%u" with "Background" instead as we always have exactly one thread running background tasks.
Description was changed from ========== content: Depict priority in raster worker thread's name. Adding priority in raster worker thread's name distinguishes different tile worker threads in traces, which will help analyse the raster tasks run by those threads to further level. The normal priority thread is prefixed with "CompositorTileWorker", while background priority thread is prefixed with "CompositorBackgroundTileWorker". However thread number in prefix is kept increasing only. BUG=None ========== to ========== content: Depict priority in raster worker thread's name. Adding priority in raster worker thread's name distinguishes different tile worker threads in traces, which will help analyse the raster tasks run by those threads to further level. The normal priority thread is prefixed with "CompositorTileWorker", while background priority thread is prefixed with "CompositorTileWorkerBackground". BUG=None ==========
On 2016/03/25 06:30:02, reveman wrote: > On 2016/03/25 at 06:19:15, prashant.n wrote: > > One query, not related to this patch - why raster_worker_pool is in > content/renderer and not in cc/raster? > > This is used for more than raster these days. It's used by media/ for video > decode. These threads and the class itself is not named properly anymore. We > should probably rename it to something like "PrioritizedWorkerPool" or > "CategorizedWorkerPool" with matching thread name changes. > Hmm. Then we should move it to base/ and have generic name. > > https://codereview.chromium.org/1830693004/diff/20001/content/renderer/raster... > content/renderer/raster_worker_pool.cc:158: > base::StringPrintf("CompositorBackgroundTileWorker%u", > Maybe replace the "%u" with "Background" instead as we always have exactly one > thread running background tasks. Done.
The CQ bit was checked by prashant.n@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1830693004/#ps40001 (title: "review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1830693004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1830693004/40001
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...)
prashant.n@samsung.com changed reviewers: + esprehn@chromium.org - jam@chromium.org
ptal
jam@chromium.org changed reviewers: + jam@chromium.org
lgtm
The CQ bit was checked by prashant.n@samsung.com to run a CQ dry run
The CQ bit was unchecked by prashant.n@samsung.com
The CQ bit was checked by prashant.n@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1830693004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1830693004/40001
Message was sent while issue was closed.
Description was changed from ========== content: Depict priority in raster worker thread's name. Adding priority in raster worker thread's name distinguishes different tile worker threads in traces, which will help analyse the raster tasks run by those threads to further level. The normal priority thread is prefixed with "CompositorTileWorker", while background priority thread is prefixed with "CompositorTileWorkerBackground". BUG=None ========== to ========== content: Depict priority in raster worker thread's name. Adding priority in raster worker thread's name distinguishes different tile worker threads in traces, which will help analyse the raster tasks run by those threads to further level. The normal priority thread is prefixed with "CompositorTileWorker", while background priority thread is prefixed with "CompositorTileWorkerBackground". BUG=None ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== content: Depict priority in raster worker thread's name. Adding priority in raster worker thread's name distinguishes different tile worker threads in traces, which will help analyse the raster tasks run by those threads to further level. The normal priority thread is prefixed with "CompositorTileWorker", while background priority thread is prefixed with "CompositorTileWorkerBackground". BUG=None ========== to ========== content: Depict priority in raster worker thread's name. Adding priority in raster worker thread's name distinguishes different tile worker threads in traces, which will help analyse the raster tasks run by those threads to further level. The normal priority thread is prefixed with "CompositorTileWorker", while background priority thread is prefixed with "CompositorTileWorkerBackground". BUG=None Committed: https://crrev.com/af5e7fd4d6d8a97444ef0e605491b24be08d5adf Cr-Commit-Position: refs/heads/master@{#383703} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/af5e7fd4d6d8a97444ef0e605491b24be08d5adf Cr-Commit-Position: refs/heads/master@{#383703} |
