Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577843002:
Add UMAs for GL context memory usage (Closed)
DescriptionAdd UMAs for GL context memory usage
Adds 6 new UMA metrics which log GL context memory. Memory is logged separately for WebGL and GLES contexts. Additionally, logging occurs at three points - periodically (every 1s), at shutdown, and when the GPU process gets a CRITICAL memory pressure signal. The new histograms are:
GPU.ContextMemory.WebGL.Periodic
GPU.ContextMemory.WebGL.Shutdown
GPU.ContextMemory.WebGL.Pressure
GPU.ContextMemory.GLES.Periodic
GPU.ContextMemory.GLES.Shutdown
GPU.ContextMemory.GLES.Pressure
BUG=667013
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/b10572ad91a13dfe5b6faef18990d8464714e666
Cr-Commit-Position: refs/heads/master@{#440882}
Patch Set 1 #Patch Set 2 : cleanup and xml #Patch Set 3 : cleanup #Patch Set 4 : rebase #
Total comments: 4
Patch Set 5 : 30s periodic dump timer #
Total comments: 4
Patch Set 6 : feedback #
Total comments: 2
Patch Set 7 : s/mb/MB and update pretty_print.py #
Messages
Total messages: 36 (21 generated)
Description was changed from ========== Add UMA for WebGL memory usage BUG= ========== to ========== Add UMA for WebGL memory usage BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
cleanup and xml
Description was changed from ========== Add UMA for WebGL memory usage BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add UMAs for GL context memory usage Adds 6 new UMA metrics which log GL context memory. Memory is logged separately for WebGL and GLES contexts. Additionally, logging occurs at three points - periodically (every 1s), at shutdown, and when the GPU process gets a CRITICAL memory pressure signal. The new histograms are: GPU.ContextMemory.WebGL.Periodic GPU.ContextMemory.WebGL.Shutdown GPU.ContextMemory.WebGL.Pressure GPU.ContextMemory.GLES.Periodic GPU.ContextMemory.GLES.Shutdown GPU.ContextMemory.GLES.Pressure BUG=667013 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
rebase
ericrk@chromium.org changed reviewers: + kbr@chromium.org
https://codereview.chromium.org/2577843002/diff/60001/gpu/ipc/service/gpu_com... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2577843002/diff/60001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:109: memory_pressure_listener_(new base::MemoryPressureListener( Unfortunately, memory_pressure_listener_ doesn't support a custom TaskRunner - instead it always calls back on the same thread it was created on. CommandBufferMemoryTracker appears to be created on the provided task_runner's thread, so I think we're OK here.
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks very much for picking this up Eric. The code overall looks fine, but one concern. https://codereview.chromium.org/2577843002/diff/60001/gpu/ipc/service/gpu_com... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2577843002/diff/60001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:112: // Set up |memory_stats_timer_| to call LogMemoryPeriodic every 1s Sampling every 1s sounds excessive to me. I think this has the potential to visibly and negatively impact high performance applications. Could we please consider (a) increasing it to something like 30s, (b) doing a one-shot earlier than 30s and having something like a 30s interval afterward, or (c) something different?
https://codereview.chromium.org/2577843002/diff/60001/gpu/ipc/service/gpu_com... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2577843002/diff/60001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:112: // Set up |memory_stats_timer_| to call LogMemoryPeriodic every 1s On 2016/12/20 03:55:39, Ken Russell wrote: > Sampling every 1s sounds excessive to me. I think this has the potential to > visibly and negatively impact high performance applications. Could we please > consider (a) increasing it to something like 30s, (b) doing a one-shot earlier > than 30s and having something like a 30s interval afterward, or (c) something > different? Let's just do 30s... If a context lasts less than 30s, the shutdown histogram will catch the data. I guess we could dump shutdown data to the "periodic" histogram in cases where shutdown happened before a single periodic dump? but that seems like overkill for the complexity added. WDYT?
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kbr@chromium.org changed reviewers: + isherman@chromium.org, jwd@chromium.org
Thanks again. LGTM. Couple questions. Also, you'll need an OWNERS review -- CC'ing a couple of tools/metrics OWNERS. https://codereview.chromium.org/2577843002/diff/60001/gpu/ipc/service/gpu_com... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2577843002/diff/60001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:112: // Set up |memory_stats_timer_| to call LogMemoryPeriodic every 1s On 2016/12/20 20:01:49, ericrk wrote: > On 2016/12/20 03:55:39, Ken Russell wrote: > > Sampling every 1s sounds excessive to me. I think this has the potential to > > visibly and negatively impact high performance applications. Could we please > > consider (a) increasing it to something like 30s, (b) doing a one-shot earlier > > than 30s and having something like a 30s interval afterward, or (c) something > > different? > > Let's just do 30s... If a context lasts less than 30s, the shutdown histogram > will catch the data. I guess we could dump shutdown data to the "periodic" > histogram in cases where shutdown happened before a single periodic dump? but > that seems like overkill for the complexity added. WDYT? Yes, sounds like overkill. 30s sounds great. Thanks. https://codereview.chromium.org/2577843002/diff/80001/gpu/ipc/service/gpu_com... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2577843002/diff/80001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:112: // Set up |memory_stats_timer_| to call LogMemoryPeriodic every 1s Update comment to indicate correct time, or just remove the reference to the exact time. https://codereview.chromium.org/2577843002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2577843002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:110000: + <affected-histogram name="GPU.ContextMemory.WebGL"/> Should the two affected-histogram lines here go side-by-side for clarity?
https://codereview.chromium.org/2577843002/diff/80001/gpu/ipc/service/gpu_com... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2577843002/diff/80001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:112: // Set up |memory_stats_timer_| to call LogMemoryPeriodic every 1s On 2016/12/20 20:28:03, Ken Russell wrote: > Update comment to indicate correct time, or just remove the reference to the > exact time. Done. https://codereview.chromium.org/2577843002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2577843002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:110000: + <affected-histogram name="GPU.ContextMemory.WebGL"/> On 2016/12/20 20:28:03, Ken Russell wrote: > Should the two affected-histogram lines here go side-by-side for clarity? Yeah, no idea why I did it like that :/
Metrics LGTM, thanks. https://codereview.chromium.org/2577843002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2577843002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:19780: +<histogram name="GPU.ContextMemory" units="mb"> nit: s/mb/MB (and if you have a chance, could you please also update the UNIT_REWRITES at the top of pretty_print.py?)
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2577843002/#ps120001 (title: "s/mb/MB and update pretty_print.py")
https://codereview.chromium.org/2577843002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2577843002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:19780: +<histogram name="GPU.ContextMemory" units="mb"> On 2016/12/20 22:08:51, Ilya Sherman (Away De.29-Ja.4) wrote: > nit: s/mb/MB (and if you have a chance, could you please also update the > UNIT_REWRITES at the top of pretty_print.py?) Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)
On 2016/12/21 05:07:26, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...) Not sure why the AMD bots didn't have capacity last night, but right now they're affected by http://crbug.com/676333 . I recommend waiting until that's resolved, but you could use NOTRY=true if you needed to.
The CQ bit was checked by ericrk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482951841006810,
"parent_rev": "7780eda3fe6ace8490c089e071ae2260b671610b", "commit_rev":
"0aa645f31980f956311f79b090b9c719a23acbc7"}
Message was sent while issue was closed.
Description was changed from ========== Add UMAs for GL context memory usage Adds 6 new UMA metrics which log GL context memory. Memory is logged separately for WebGL and GLES contexts. Additionally, logging occurs at three points - periodically (every 1s), at shutdown, and when the GPU process gets a CRITICAL memory pressure signal. The new histograms are: GPU.ContextMemory.WebGL.Periodic GPU.ContextMemory.WebGL.Shutdown GPU.ContextMemory.WebGL.Pressure GPU.ContextMemory.GLES.Periodic GPU.ContextMemory.GLES.Shutdown GPU.ContextMemory.GLES.Pressure BUG=667013 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Add UMAs for GL context memory usage Adds 6 new UMA metrics which log GL context memory. Memory is logged separately for WebGL and GLES contexts. Additionally, logging occurs at three points - periodically (every 1s), at shutdown, and when the GPU process gets a CRITICAL memory pressure signal. The new histograms are: GPU.ContextMemory.WebGL.Periodic GPU.ContextMemory.WebGL.Shutdown GPU.ContextMemory.WebGL.Pressure GPU.ContextMemory.GLES.Periodic GPU.ContextMemory.GLES.Shutdown GPU.ContextMemory.GLES.Pressure BUG=667013 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2577843002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add UMAs for GL context memory usage Adds 6 new UMA metrics which log GL context memory. Memory is logged separately for WebGL and GLES contexts. Additionally, logging occurs at three points - periodically (every 1s), at shutdown, and when the GPU process gets a CRITICAL memory pressure signal. The new histograms are: GPU.ContextMemory.WebGL.Periodic GPU.ContextMemory.WebGL.Shutdown GPU.ContextMemory.WebGL.Pressure GPU.ContextMemory.GLES.Periodic GPU.ContextMemory.GLES.Shutdown GPU.ContextMemory.GLES.Pressure BUG=667013 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2577843002 ========== to ========== Add UMAs for GL context memory usage Adds 6 new UMA metrics which log GL context memory. Memory is logged separately for WebGL and GLES contexts. Additionally, logging occurs at three points - periodically (every 1s), at shutdown, and when the GPU process gets a CRITICAL memory pressure signal. The new histograms are: GPU.ContextMemory.WebGL.Periodic GPU.ContextMemory.WebGL.Shutdown GPU.ContextMemory.WebGL.Pressure GPU.ContextMemory.GLES.Periodic GPU.ContextMemory.GLES.Shutdown GPU.ContextMemory.GLES.Pressure BUG=667013 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/b10572ad91a13dfe5b6faef18990d8464714e666 Cr-Commit-Position: refs/heads/master@{#440882} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b10572ad91a13dfe5b6faef18990d8464714e666 Cr-Commit-Position: refs/heads/master@{#440882} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
