Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2047533002:
[tracing] Add browser test for background memory tracing (Closed)
|
Created:
4 years, 6 months ago by ssid Modified:
4 years, 6 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, imcheng+watch_chromium.org, wfh+watch_chromium.org, avayvod+watch_chromium.org, jasonroberts+watch_google.com, tracing+reviews_chromium.org, chromium-apps-reviews_chromium.org, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org, Maria Base URL:
https://chromium.googlesource.com/chromium/src.git@set_whitelist Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Add browser test for background memory tracing
This CL adds a browser test for background mode memory tracing.
To start tracing in background mode, trace config string must be passed
with background mode trigger. So, this CL edits the tracing test to
support trace config strings.
This CL also removes the single process test for tracing since single
process mode is not supported well.
Discardable dump provider creates dumps with same name from browser and
renderer and this is not allowed in single process mode. So, child ids
are added to the dumps.
BUG=613198
Committed: https://crrev.com/c442e187b9c818fe715817b332bbb1f3aaa38301
Cr-Commit-Position: refs/heads/master@{#400333}
Patch Set 1 : Rebase. #Patch Set 2 : Remove single proc tests. #
Total comments: 6
Patch Set 3 : Fixes. #
Total comments: 4
Patch Set 4 : Remove useless watch event. #
Depends on Patchset: Messages
Total messages: 46 (26 generated)
Description was changed from ========== [tracing] Add background memory tracing browser test This CL adds a browser test for background mode memory tracing. To start tracing in background mode, trace config string must be passed with background mode trigger. So, this CL edits the tracing test to support trace config strings. BUG=613198 ========== to ========== [tracing] Add background memory tracing browser test This CL adds a browser test for background mode memory tracing. To start tracing in background mode, trace config string must be passed with background mode trigger. So, this CL edits the tracing test to support trace config strings. Discardable dump provider creates dumps with same name from browser and renderer and this is not allowed in single process mode. So, child ids are added to the dumps. BUG=613198 ==========
Patchset #6 (id:100001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
ssid@chromium.org changed reviewers: + primiano@chromium.org
This adds tests for the CL: https://codereview.chromium.org/2012883003/ Test ensures 2 things: The dump providers do not crash. They also do not create dumps with wrong names. It is better to have test on waterfall that breaks for errors. This should also answer your question about discardable memory in the other CL. I forgot about single process mode and this CL a fixes the issue, while the preceding CL already holds whitelist values for this change. The performance tracking will be addressed in next CL which adds telemetry benchmark: https://codereview.chromium.org/2052753002/
Patchset #1 (id:120001) has been deleted
Patchset #2 (id:160001) has been deleted
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047533002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
Description was changed from ========== [tracing] Add background memory tracing browser test This CL adds a browser test for background mode memory tracing. To start tracing in background mode, trace config string must be passed with background mode trigger. So, this CL edits the tracing test to support trace config strings. Discardable dump provider creates dumps with same name from browser and renderer and this is not allowed in single process mode. So, child ids are added to the dumps. BUG=613198 ========== to ========== [tracing] Add browser test for background memory tracing This CL adds a browser test for background mode memory tracing. To start tracing in background mode, trace config string must be passed with background mode trigger. So, this CL edits the tracing test to support trace config strings. This CL also removes the single process test for tracing since single process mode is not supported well. Discardable dump provider creates dumps with same name from browser and renderer and this is not allowed in single process mode. So, child ids are added to the dumps. BUG=613198 ==========
Patchset #2 (id:180001) has been deleted
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047533002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
From discussion with Maria and Bo, it seems like --single-process is not the right way to test functions on WebView, since single process flag is not supported well in chromium. I am not sure how webview is actually run. Due to this single-process flag we have been getting a lot unrelated bugs on this test like crbug.com/594884, crbug.com/585026. These bugs are not a priority because the teams do not wish to support single-process mode. Maybe we should have an integration test, but at that point sounds like telemetry is the right thing to do and we already have a telemetry. This is the reason I decided to remove these tests. Even in the GPU process bug crbug.com/547033 or crbug.com/619515 happens only in single process mode. What do you think?
On 2016/06/14 17:16:18, ssid wrote: > From discussion with Maria and Bo, it seems like --single-process is not the > right way to test functions on WebView, since single process flag is not > supported well in chromium. I am not sure how webview is actually run. Due to > this single-process flag we have been getting a lot unrelated bugs on this test > like crbug.com/594884, crbug.com/585026. > These bugs are not a priority because the teams do not wish to support > single-process mode. Maybe we should have an integration test, but at that point > sounds like telemetry is the right thing to do and we already have a telemetry. > This is the reason I decided to remove these tests. Even in the GPU process bug > crbug.com/547033 or crbug.com/619515 happens only in single process mode. > What do you think? Removing the SP and the new test LG. Not sure if you really need all those refactorings instead of just keeping the category ctor?
apparently forgot to send comments in the previous email https://codereview.chromium.org/2047533002/diff/220001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2047533002/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:1151: TraceConfigMemoryTestUtil::GetTraceConfig_BackgroundTrigger(1)); 1 /* period */ https://codereview.chromium.org/2047533002/diff/220001/base/trace_event/trace... File base/trace_event/trace_config_memory_test_util.h (right): https://codereview.chromium.org/2047533002/diff/220001/base/trace_event/trace... base/trace_event/trace_config_memory_test_util.h:75: static std::string GetTraceConfig_BackgroundTrigger(int period) { maybe s/period/period_ms/ https://codereview.chromium.org/2047533002/diff/220001/chrome/test/base/traci... File chrome/test/base/tracing.h (right): https://codereview.chromium.org/2047533002/diff/220001/chrome/test/base/traci... chrome/test/base/tracing.h:22: bool BeginTracing(const base::trace_event::TraceConfig& trace_config) If you kept the old version would have required less owners to review :) Happy in both cases really. Consider that below you still have the version with category_patterns, so you don't have the argument of "for consistency, let's use the new TraceConfig everywhere", which would have won. Anyways, happy in both cases, this is a minor thing really.
https://codereview.chromium.org/2047533002/diff/220001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2047533002/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:1151: TraceConfigMemoryTestUtil::GetTraceConfig_BackgroundTrigger(1)); On 2016/06/15 14:19:57, Primiano Tucci wrote: > 1 /* period */ Done. https://codereview.chromium.org/2047533002/diff/220001/base/trace_event/trace... File base/trace_event/trace_config_memory_test_util.h (right): https://codereview.chromium.org/2047533002/diff/220001/base/trace_event/trace... base/trace_event/trace_config_memory_test_util.h:75: static std::string GetTraceConfig_BackgroundTrigger(int period) { On 2016/06/15 14:19:57, Primiano Tucci wrote: > maybe s/period/period_ms/ Done. https://codereview.chromium.org/2047533002/diff/220001/chrome/test/base/traci... File chrome/test/base/tracing.h (right): https://codereview.chromium.org/2047533002/diff/220001/chrome/test/base/traci... chrome/test/base/tracing.h:22: bool BeginTracing(const base::trace_event::TraceConfig& trace_config) On 2016/06/15 14:19:57, Primiano Tucci wrote: > If you kept the old version would have required less owners to review :) > Happy in both cases really. > Consider that below you still have the version with category_patterns, so you > don't have the argument of "for consistency, let's use the new TraceConfig > everywhere", which would have won. > Anyways, happy in both cases, this is a minor thing really. Yeah I wanted to change that one, but too big a change. It makes sense to just add another function here
ssid@chromium.org changed reviewers: + sievers@chromium.org, thestig@chromium.org
+Lei Please look at the changes in the browser tests. +sievers for a small tracing change in content/ similar to https://codereview.chromium.org/2067543003/ Thanks!
On 2016/06/15 17:55:45, ssid wrote: > +Lei Please look at the changes in the browser tests. > > +sievers for a small tracing change in content/ similar to > https://codereview.chromium.org/2067543003/ > > Thanks! lgtm
On 2016/06/15 17:55:45, ssid wrote: > +Lei Please look at the changes in the browser tests. I defer to primiano and also wrote https://codereview.chromium.org/2069973003/ to better define OWNERship for these files.
LGTM with some comments https://codereview.chromium.org/2047533002/diff/240001/chrome/test/base/traci... File chrome/test/base/tracing_browsertest.cc (right): https://codereview.chromium.org/2047533002/diff/240001/chrome/test/base/traci... chrome/test/base/tracing_browsertest.cc:162: GetTraceConfig_PeriodicTriggers(250, 2000))); aren't this value going to make this test too slow? Isn't this going to make the test wait 2s? https://codereview.chromium.org/2047533002/diff/240001/content/child/child_di... File content/child/child_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2047533002/diff/240001/content/child/child_di... content/child/child_discardable_shared_memory_manager.cc:223: pmd->CreateAllocatorDump( do you intend to do this change here (is it a piece you forgot from the other cl) or is this accidental?
rs lgtm
Patchset #5 (id:260001) has been deleted
Thanks. https://codereview.chromium.org/2047533002/diff/240001/chrome/test/base/traci... File chrome/test/base/tracing_browsertest.cc (right): https://codereview.chromium.org/2047533002/diff/240001/chrome/test/base/traci... chrome/test/base/tracing_browsertest.cc:162: GetTraceConfig_PeriodicTriggers(250, 2000))); On 2016/06/16 13:32:30, Primiano Tucci wrote: > aren't this value going to make this test too slow? Isn't this going to make the > test wait 2s? These were the existing values (when enabled just with category name). Note that we don't wait for any particular dump to finish / get called. This just tests if the dumps do not crash. This should not affect the test duration. Also, the wait for watch event should have been removed. Fixed it. It was anyway waiting for any trace event with memory-infra category, which is not very useful. https://codereview.chromium.org/2047533002/diff/240001/content/child/child_di... File content/child/child_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/2047533002/diff/240001/content/child/child_di... content/child/child_discardable_shared_memory_manager.cc:223: pmd->CreateAllocatorDump( On 2016/06/16 13:32:30, Primiano Tucci wrote: > do you intend to do this change here (is it a piece you forgot from the other > cl) or is this accidental? I intend to change this with this cl because it fixes the test. This actually edits the dump names, not just fixes the format. It used to give same dump names for all renderers, which is bad for single process mode.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047533002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, thestig@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2047533002/#ps280001 (title: "Remove useless watch event.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047533002/280001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Add browser test for background memory tracing This CL adds a browser test for background mode memory tracing. To start tracing in background mode, trace config string must be passed with background mode trigger. So, this CL edits the tracing test to support trace config strings. This CL also removes the single process test for tracing since single process mode is not supported well. Discardable dump provider creates dumps with same name from browser and renderer and this is not allowed in single process mode. So, child ids are added to the dumps. BUG=613198 ========== to ========== [tracing] Add browser test for background memory tracing This CL adds a browser test for background mode memory tracing. To start tracing in background mode, trace config string must be passed with background mode trigger. So, this CL edits the tracing test to support trace config strings. This CL also removes the single process test for tracing since single process mode is not supported well. Discardable dump provider creates dumps with same name from browser and renderer and this is not allowed in single process mode. So, child ids are added to the dumps. BUG=613198 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Add browser test for background memory tracing This CL adds a browser test for background mode memory tracing. To start tracing in background mode, trace config string must be passed with background mode trigger. So, this CL edits the tracing test to support trace config strings. This CL also removes the single process test for tracing since single process mode is not supported well. Discardable dump provider creates dumps with same name from browser and renderer and this is not allowed in single process mode. So, child ids are added to the dumps. BUG=613198 ========== to ========== [tracing] Add browser test for background memory tracing This CL adds a browser test for background mode memory tracing. To start tracing in background mode, trace config string must be passed with background mode trigger. So, this CL edits the tracing test to support trace config strings. This CL also removes the single process test for tracing since single process mode is not supported well. Discardable dump provider creates dumps with same name from browser and renderer and this is not allowed in single process mode. So, child ids are added to the dumps. BUG=613198 Committed: https://crrev.com/c442e187b9c818fe715817b332bbb1f3aaa38301 Cr-Commit-Position: refs/heads/master@{#400333} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c442e187b9c818fe715817b332bbb1f3aaa38301 Cr-Commit-Position: refs/heads/master@{#400333}
Message was sent while issue was closed.
Patchset #2 (id:200001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
