Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1266833004:
telemetry: Add a page set for blink's memory usage measurement (Closed)
Descriptiontelemetry: Add a page set for blink's memory usage measurement
The page set will be used as metrics for tracking memory
reduction efforts on Blink. Details can be found at [1].
[1] https://docs.google.com/document/d/1zlGQkwkWEu5LUg-CrZHHRuhDtZcuvrRv8bjdC8-noaU/edit?usp=sharing
BUG=524338
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
Committed: https://crrev.com/0836ebab5ec66a9092722acacdbacc15b02b18ce
Cr-Commit-Position: refs/heads/master@{#348864}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Preparing for recording #
Total comments: 2
Patch Set 7 : Ready for review #
Total comments: 16
Patch Set 8 : #
Total comments: 51
Patch Set 9 : Timeline based #
Total comments: 12
Patch Set 10 : #Patch Set 11 : #
Total comments: 2
Patch Set 12 : #
Total comments: 10
Patch Set 13 : #
Total comments: 2
Patch Set 14 : #
Total comments: 7
Patch Set 15 : #
Total comments: 1
Patch Set 16 : #Patch Set 17 : Pass no-sandbox #Patch Set 18 : Disabled #
Messages
Total messages: 119 (31 generated)
bashi@chromium.org changed reviewers: + petrcermak@chromium.org, primiano@chromium.org
Hi petrcermak, I'm trying to use request-based memory dumps you added in https://codereview.chromium.org/1224083015, but I cannot get the requested dump in some cases. Do you have any idea about the cause? Thanks, https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... File tools/perf/measurements/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... tools/perf/measurements/renderer_memory.py:41: assert len(memory_dumps) == 1 I hit this assert. How can I make sure that the requested dump is actually in IterGlobalMemoryDumps()?
Hi, The issue occurred to me one or two times when I started investigating it, but I have not been able to reproduce it since then (I ran 50 tests on both N9 and N5 with zero failures). Nevertheless, I think I know what the problem is: A request for a memory dump fails if there is currently another memory dump in progress (see https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr...). Since your test doesn't disable periodic memory dumps (which are automatically enabled by the 'disabled-by-default-memory-infra' tracing category), it's possible that your on-demand memory dump request clashes with the periodic one, which causes it to fail. As I have explained in the inline comments, you can disable periodic memory dumps by passing the '--enable-memory-benchmarking' command-line flag to the browser. This will hopefully solve your problems. Let me know if it helped. [Note that using the command-line flag to disable periodic memory dumps is a temporary workaround that will hopefully be fixed soon by extending the TraceConfig (see crbug.com/513692).] Thanks, Petr P.S. It's great to see other teams use our tools. Feel free to contact me if you have any other questions/ideas/comments. https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... File tools/perf/measurements/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... tools/perf/measurements/renderer_memory.py:26: options.AppendExtraBrowserArgs(['--no-sandbox']) You need to add '--enable-memory-benchmarking' here to disable periodic memory dumps. https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... tools/perf/measurements/renderer_memory.py:30: guid = tab.browser.DumpMemory() I suggest you add an assert below this statement: assert guid is not None https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... tools/perf/measurements/renderer_memory.py:40: if dump.dump_id == guid] You won't need to do any filtering once you disable the periodic dumps via the extra command-line flag: memory_dumps = list(timeline_model.IterGlobalMemoryDumps()) assert len(memory_dumps) == 1 assert memory_dumps[0].dump_id == guid https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... tools/perf/measurements/renderer_memory.py:41: assert len(memory_dumps) == 1 On 2015/08/04 06:44:23, bashi1 wrote: > I hit this assert. How can I make sure that the requested dump is actually in > IterGlobalMemoryDumps()? You should not need to do this (the dump should always be there when you disable the periodic dumps), but it could be done as follows (again, you should not need to do this): for _ in xrange(MAX_RETRIES): guid = tab.browser.DumpMemory() if guid is not None: break else: raise Exception('Failed to dump memory')
On 2015/08/04 13:05:03, petrcermak wrote: > Hi, > > The issue occurred to me one or two times when I started investigating it, but I > have not been able to reproduce it since then (I ran 50 tests on both N9 and N5 > with zero failures). > > Nevertheless, I think I know what the problem is: A request for a memory dump > fails if there is currently another memory dump in progress (see > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr...). > Since your test doesn't disable periodic memory dumps (which are automatically > enabled by the 'disabled-by-default-memory-infra' tracing category), it's > possible that your on-demand memory dump request clashes with the periodic one, > which causes it to fail. > > As I have explained in the inline comments, you can disable periodic memory > dumps by passing the '--enable-memory-benchmarking' command-line flag to the > browser. This will hopefully solve your problems. Let me know if it helped. > > [Note that using the command-line flag to disable periodic memory dumps is a > temporary workaround that will hopefully be fixed soon by extending the > TraceConfig (see crbug.com/513692).] > > Thanks, > Petr Thanks for the detailed explanation for the workaround. It works. The CL itself is still WIP (I need to update outdated page sets) but I can get consistent results now. > > P.S. It's great to see other teams use our tools. Feel free to contact me if you > have any other questions/ideas/comments. :) > > https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... > File tools/perf/measurements/renderer_memory.py (right): > > https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... > tools/perf/measurements/renderer_memory.py:26: > options.AppendExtraBrowserArgs(['--no-sandbox']) > You need to add '--enable-memory-benchmarking' here to disable periodic memory > dumps. > > https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... > tools/perf/measurements/renderer_memory.py:30: guid = tab.browser.DumpMemory() > I suggest you add an assert below this statement: > > assert guid is not None > > https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... > tools/perf/measurements/renderer_memory.py:40: if dump.dump_id == guid] > You won't need to do any filtering once you disable the periodic dumps via the > extra command-line flag: > > memory_dumps = list(timeline_model.IterGlobalMemoryDumps()) > assert len(memory_dumps) == 1 > assert memory_dumps[0].dump_id == guid > > https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... > tools/perf/measurements/renderer_memory.py:41: assert len(memory_dumps) == 1 > On 2015/08/04 06:44:23, bashi1 wrote: > > I hit this assert. How can I make sure that the requested dump is actually in > > IterGlobalMemoryDumps()? > > You should not need to do this (the dump should always be there when you disable > the periodic dumps), but it could be done as follows (again, you should not need > to do this): > > for _ in xrange(MAX_RETRIES): > guid = tab.browser.DumpMemory() > if guid is not None: > break > else: > raise Exception('Failed to dump memory')
https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... File tools/perf/measurements/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... tools/perf/measurements/renderer_memory.py:26: options.AppendExtraBrowserArgs(['--no-sandbox']) On 2015/08/04 13:05:03, petrcermak wrote: > You need to add '--enable-memory-benchmarking' here to disable periodic memory > dumps. Done. https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... tools/perf/measurements/renderer_memory.py:30: guid = tab.browser.DumpMemory() On 2015/08/04 13:05:03, petrcermak wrote: > I suggest you add an assert below this statement: > > assert guid is not None Done. https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... tools/perf/measurements/renderer_memory.py:40: if dump.dump_id == guid] On 2015/08/04 13:05:03, petrcermak wrote: > You won't need to do any filtering once you disable the periodic dumps via the > extra command-line flag: > > memory_dumps = list(timeline_model.IterGlobalMemoryDumps()) > assert len(memory_dumps) == 1 > assert memory_dumps[0].dump_id == guid Done. https://codereview.chromium.org/1266833004/diff/1/tools/perf/measurements/ren... tools/perf/measurements/renderer_memory.py:41: assert len(memory_dumps) == 1 On 2015/08/04 13:05:03, petrcermak wrote: > On 2015/08/04 06:44:23, bashi1 wrote: > > I hit this assert. How can I make sure that the requested dump is actually in > > IterGlobalMemoryDumps()? > > You should not need to do this (the dump should always be there when you disable > the periodic dumps), but it could be done as follows (again, you should not need > to do this): > > for _ in xrange(MAX_RETRIES): > guid = tab.browser.DumpMemory() > if guid is not None: > break > else: > raise Exception('Failed to dump memory') Removed filtering:)
bashi@chromium.org changed reviewers: + hajimehoshi@chromium.org
Hi petrcermak, We have another question. Could you give us some advices? Thanks! https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... File tools/perf/measurements/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:38: assert guid is not None I no longer hit this assertion, but hajimehoshi@ has always hit this assertion. When guid could be None? hajimehoshi@, could you provide a trace dump?
On 2015/08/19 09:00:45, bashi1 wrote: > Hi petrcermak, > > We have another question. Could you give us some advices? > > Thanks! > > https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... > File tools/perf/measurements/renderer_memory.py (right): > > https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... > tools/perf/measurements/renderer_memory.py:38: assert guid is not None > I no longer hit this assertion, but hajimehoshi@ has always hit this assertion. > When guid could be None? > > hajimehoshi@, could you provide a trace dump? Sure. Here is the log: https://drive.google.com/a/chromium.org/file/d/0BwW8PrCcts4WTWY4UTBCSkNIdVk/v...
FYI petrcermak is OOO this week. I guess he'll look into this once back.
Finally back from vacation... https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... File tools/perf/measurements/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:38: assert guid is not None On 2015/08/19 09:00:45, bashi1 wrote: > I no longer hit this assertion, but hajimehoshi@ has always hit this assertion. > When guid could be None? GUID will be None in the following two cases: 1. Tracing was started without the 'disabled-by-default-memory-infra' category enabled. 2. A memory dump failed for some reason (e.g. when another memory dump is already being generated). Note that the DumpMemory() method will throw an exception (rather than return None) when tracing is not started at all. > hajimehoshi@, could you provide a trace dump? I can see that the attached trace contains no memory dumps at all. Which OS are you using? Could you please provide the necessary steps to reproduce the issue? Thanks
Thanks for the response! On 2015/08/24 11:57:16, petrcermak wrote: > Finally back from vacation... > > https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... > File tools/perf/measurements/renderer_memory.py (right): > > https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... > tools/perf/measurements/renderer_memory.py:38: assert guid is not None > On 2015/08/19 09:00:45, bashi1 wrote: > > I no longer hit this assertion, but hajimehoshi@ has always hit this > assertion. > > When guid could be None? > > GUID will be None in the following two cases: > > 1. Tracing was started without the 'disabled-by-default-memory-infra' category > enabled. > 2. A memory dump failed for some reason (e.g. when another memory dump is > already being generated). We specify 'disabled-by-default-memory-infra' so probably because of 2? But we also pass '--enable-memory-benchmarking' flag, which should prevent another dump... > > Note that the DumpMemory() method will throw an exception (rather than return > None) when tracing is not started at all. We haven't seen an exception when calling DumpMemory(). > > > hajimehoshi@, could you provide a trace dump? > > I can see that the attached trace contains no memory dumps at all. Which OS are > you using? Could you please provide the necessary steps to reproduce the issue? > Thanks We are running this benchmark on android. Steps: 1. apply this CL $ git cl patch 1266833004 2. build chrome apk $ ninja -C out/Release chrome_public_apk 3. run telemetry $ tools/perf/run_benchmark --reset-results --device=xxx --browser=android-chromium --use-live-sites renderer_memory.blink_memory_mobile
On 2015/08/25 07:16:00, bashi1 wrote: > Thanks for the response! > > On 2015/08/24 11:57:16, petrcermak wrote: > > Finally back from vacation... > > > > > https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... > > File tools/perf/measurements/renderer_memory.py (right): > > > > > https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... > > tools/perf/measurements/renderer_memory.py:38: assert guid is not None > > On 2015/08/19 09:00:45, bashi1 wrote: > > > I no longer hit this assertion, but hajimehoshi@ has always hit this > > assertion. > > > When guid could be None? > > > > GUID will be None in the following two cases: > > > > 1. Tracing was started without the 'disabled-by-default-memory-infra' > category > > enabled. > > 2. A memory dump failed for some reason (e.g. when another memory dump is > > already being generated). > We specify 'disabled-by-default-memory-infra' so probably because of 2? But we > also pass '--enable-memory-benchmarking' flag, which should prevent another > dump... > > > > Note that the DumpMemory() method will throw an exception (rather than return > > None) when tracing is not started at all. > We haven't seen an exception when calling DumpMemory(). > > > > > hajimehoshi@, could you provide a trace dump? > > > > I can see that the attached trace contains no memory dumps at all. Which OS > are > > you using? Could you please provide the necessary steps to reproduce the > issue? > > Thanks > > We are running this benchmark on android. Steps: > 1. apply this CL > $ git cl patch 1266833004 > 2. build chrome apk > $ ninja -C out/Release chrome_public_apk > 3. run telemetry > $ tools/perf/run_benchmark --reset-results --device=xxx > --browser=android-chromium --use-live-sites renderer_memory.blink_memory_mobile I ran the benchmark many times on N9 and I get no memory dump issues (only lots of GMail timeouts and one DevTools navigation failure on G+). Could you please run the following command and attach the generated renderer_benchmark_android.log file: $ tools/perf/run_benchmark --reset-results --device=<DEVICE-ID> --browser=android-chromium --use-live-sites renderer_memory.blink_memory_mobile --pageset-repeat=50 2>&1 | tee renderer_benchmark_android.log Thanks, Petr
On 2015/08/25 12:11:00, petrcermak wrote: > On 2015/08/25 07:16:00, bashi1 wrote: > > Thanks for the response! > > > > On 2015/08/24 11:57:16, petrcermak wrote: > > > Finally back from vacation... > > > > > > > > > https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... > > > File tools/perf/measurements/renderer_memory.py (right): > > > > > > > > > https://codereview.chromium.org/1266833004/diff/100001/tools/perf/measurement... > > > tools/perf/measurements/renderer_memory.py:38: assert guid is not None > > > On 2015/08/19 09:00:45, bashi1 wrote: > > > > I no longer hit this assertion, but hajimehoshi@ has always hit this > > > assertion. > > > > When guid could be None? > > > > > > GUID will be None in the following two cases: > > > > > > 1. Tracing was started without the 'disabled-by-default-memory-infra' > > category > > > enabled. > > > 2. A memory dump failed for some reason (e.g. when another memory dump is > > > already being generated). > > We specify 'disabled-by-default-memory-infra' so probably because of 2? But we > > also pass '--enable-memory-benchmarking' flag, which should prevent another > > dump... > > > > > > Note that the DumpMemory() method will throw an exception (rather than > return > > > None) when tracing is not started at all. > > We haven't seen an exception when calling DumpMemory(). > > > > > > > hajimehoshi@, could you provide a trace dump? > > > > > > I can see that the attached trace contains no memory dumps at all. Which OS > > are > > > you using? Could you please provide the necessary steps to reproduce the > > issue? > > > Thanks > > > > We are running this benchmark on android. Steps: > > 1. apply this CL > > $ git cl patch 1266833004 > > 2. build chrome apk > > $ ninja -C out/Release chrome_public_apk > > 3. run telemetry > > $ tools/perf/run_benchmark --reset-results --device=xxx > > --browser=android-chromium --use-live-sites > renderer_memory.blink_memory_mobile > > I ran the benchmark many times on N9 and I get no memory dump issues (only lots > of GMail timeouts and one DevTools navigation failure on G+). Could you please > run the following command and attach the generated > renderer_benchmark_android.log file: > > $ tools/perf/run_benchmark --reset-results --device=<DEVICE-ID> > --browser=android-chromium --use-live-sites renderer_memory.blink_memory_mobile > --pageset-repeat=50 2>&1 | tee renderer_benchmark_android.log > > Thanks, > Petr I cannot reproduce the issue any more, and this is not a blocking issue for hajimehoshi@, so I think we can call it WONTFIX for now. Thank you so much for your effort on this!
bashi@chromium.org changed reviewers: + skyostil@chromium.org
PS#7 is ready for review. skyostil@, could you take a look or suggest reviewers? Thanks,
A couple of comments on code style. Thanks, Petr https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... File tools/perf/measurements/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:18: '-*,blink.console,disabled-by-default-memory-infra') This should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:25: ['--enable-memory-benchmarking', '--ignore-certificate-errors', This and the next line should be indented 4 spaces (+2) It might also be a little more readable if you put each flag on a separate line. https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:33: assert tab.browser.supports_memory_dumping Any chance this could be replaced with a decorator or using skipTest() in setUp() instead (see https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... )? https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:37: assert guid is not None No reason not to put this right after line 34. https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:41: timeline_model, guid, renderer_process.pid) This should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:49: dump for dump in memory_dumps[0].IterProcessMemoryDumps() This and the next line should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:56: def _AddAllocatorStats(category): nit: I think it would be more readable if you put a blank line above this. https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:58: results.current_page, category, 'MiB', Lines 58-60 and also 69-71 should be indented 4 spaces (+2)
Thanks for review! BTW, some bots are failing. --- Archive data file not found for /b/build/slave/android/build/src/tools/perf/page_sets/blink_memory_mobile.py I follow https://www.chromium.org/developers/telemetry/upload_to_cloud_storage#TOC-Upl... and uploaded to cloud storage. What am I missing? https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... File tools/perf/measurements/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:18: '-*,blink.console,disabled-by-default-memory-infra') On 2015/08/26 08:20:18, petrcermak wrote: > This should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:25: ['--enable-memory-benchmarking', '--ignore-certificate-errors', On 2015/08/26 08:20:18, petrcermak wrote: > This and the next line should be indented 4 spaces (+2) > > It might also be a little more readable if you put each flag on a separate line. Done. https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:33: assert tab.browser.supports_memory_dumping On 2015/08/26 08:20:18, petrcermak wrote: > Any chance this could be replaced with a decorator or using skipTest() in > setUp() instead (see > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > )? I didn't know that. Thanks! Done. https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:37: assert guid is not None On 2015/08/26 08:20:18, petrcermak wrote: > No reason not to put this right after line 34. Done. https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:41: timeline_model, guid, renderer_process.pid) On 2015/08/26 08:20:18, petrcermak wrote: > This should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:49: dump for dump in memory_dumps[0].IterProcessMemoryDumps() On 2015/08/26 08:20:18, petrcermak wrote: > This and the next line should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:56: def _AddAllocatorStats(category): On 2015/08/26 08:20:18, petrcermak wrote: > nit: I think it would be more readable if you put a blank line above this. Done. https://codereview.chromium.org/1266833004/diff/120001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:58: results.current_page, category, 'MiB', On 2015/08/26 08:20:18, petrcermak wrote: > Lines 58-60 and also 69-71 should be indented 4 spaces (+2) Done.
I completely skipped some of the files for some reason. Here are a few more code style comments. Thanks, Petr https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... File tools/perf/measurements/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:25: ['--enable-memory-benchmarking', nit: I think this would be even better with the list starting on line 24 (like here: https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...). https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:35: self.skipTest('Browser does not support memory dumping, skipping test.') I've just investigated this method a little more and it turns out that it's only available in unittests and not here (I naively assumed that PageTest is a subclass of Python's unittest.TestCase). Unfortunately, it seems that even the decorators for enabling and disabling test cases are only available to actual tests. So I guess your original assert solution was right after all. Sorry for the unnecessary extra work :-( https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:4: from telemetry.page import page as page_module nit: Please put a blank line above this one. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:15: url=url, page_set=page_set, name=name, Lines 15-17 should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:27: # pylint: disable=C0301 lines 27-30 should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:33: comment_link_selector = '.show_comments_link' This should probably be a class constant https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:36: 'window.location.hash = "comments"') This line should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:39: 'window.Chorus.Comments.collection.length > 0') This line should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:45: url='https://mail.google.com/mail/', Lines 45-47 should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:56: 'document.querySelector("#apploadingdiv").style.opacity == "0"') This line should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:64: archive_data_file='data/blink_memory_mobile.json', Lines 64-65 should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:67: # Why: High rate of Blink's memory consumption rate. I don't understand what's the difference between "High rate of Blink's memory consumption rate" and "Renderer memory usage is high". Is the former just more specific? Why is it not "Blink memory usage is high" (consistency)? https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:69: url='https://www.pinterest.com', Lines 69-71 should be indented 4 spaces (+2) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:78: url='http://en.m.wikipedia.org/wiki/Wikipedia', ditto (spaces) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:82: url='http://www.reddit.com/r/programming/comments/1g96ve', ditto (spaces) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:86: # pylint: disable=line-too-long ditto (spaces) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:93: url='http://worldjournal.com/', ditto (spaces) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:101: url='http://googlewebmastercentral.blogspot.com/2015/04/rolling-out-mobile-friendly-update.html?m=1', ditto (spaces) https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:105: # pylint: disable=line-too-long ditto (spaces)
Thanks, added a few comments. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... File tools/perf/measurements/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. Could you please add a unit test for this measurement? Check the other measurements for common ways to do this. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:21: tab.browser.platform.tracing_controller.Start(options, category_filter, 10) nit: 10s is already the default timeout so you could leave it out from here. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:18: self.arcive_data_file = 'data/blink_memory_mobile.json' typo: archive_data_file https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:24: class TheVergePage(BlinkMemoryMobilePage): Since this isn't enabled yet could we leave it out of this patch? https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:28: url='http://www.theverge.com/2015/8/11/9133883/taylor-swift-spotify-discover-weekly-what-is-going-on', Looks like this line is too long. Tip: in python you can split strings across lines like this: long_string = ('foo' 'bar') https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:53: action_runner.WaitForNavigate() RunNavigateSteps already waits for the navigation to complete so there's no need to do it again here. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:86: # pylint: disable=line-too-long On 2015/08/26 09:33:50, petrcermak wrote: > ditto (spaces) Also please break the long line instead of turning off the linter.
My suggestion here is to loop in perezju who developed / is developing the system health plan benchmark, which seems very similar to this one.
perezju@chromium.org changed reviewers: + perezju@chromium.org
On 2015/08/26 at 10:27:58, primiano wrote: > My suggestion here is to loop in perezju who developed / is developing the system health plan benchmark, which seems very similar to this one. AFAIK, the preferred method for using timeline (tracing) based metrics is to create a benchmark that overrides CreateTimelineBasedMeasurementOptions, so that all of the tracing stuff is taken care of for you. For an example have a look at: https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/benchma... https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/page_se... (just ignore the BackgroundPage bits)
nednguyen@google.com changed reviewers: + nednguyen@google.com
https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... File tools/perf/measurements/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... tools/perf/measurements/renderer_memory.py:12: class RendererMemory(page_test.PageTest): I am not ok with adding another page_test. Please use timline based measurement for your metric need. You can ask perezju@ for how.
On 2015/08/26 11:50:23, nednguyen wrote: > https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... > File tools/perf/measurements/renderer_memory.py (right): > > https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... > tools/perf/measurements/renderer_memory.py:12: class > RendererMemory(page_test.PageTest): > I am not ok with adding another page_test. Please use timline based measurement > for your metric need. You can ask perezju@ for how. Could you elaborate why you are not ok with adding a page test? Actually, we don't want to have timeline based measurement. We want to have a snapshot of memory dump at an arbitrary point. That's why I use tab.browser.DumpMemory(). Could you suggest an alternative if this isn't acceptable?
On 2015/08/26 at 23:08:50, bashi wrote: > On 2015/08/26 11:50:23, nednguyen wrote: > > https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... > > File tools/perf/measurements/renderer_memory.py (right): > > > > https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... > > tools/perf/measurements/renderer_memory.py:12: class > > RendererMemory(page_test.PageTest): > > I am not ok with adding another page_test. Please use timline based measurement > > for your metric need. You can ask perezju@ for how. > > Could you elaborate why you are not ok with adding a page test? Actually, we don't want to have timeline based measurement. We want to have a snapshot of memory dump at an arbitrary point. That's why I use tab.browser.DumpMemory(). Could you suggest an alternative if this isn't acceptable? Requesting memory dumps at arbitrary points is supported by timeline based measurements. The memory_health_story.py I linked to, in fact, uses tab.browser.DumpMemory() to request dumps.
On 2015/08/27 08:25:09, perezju wrote: > On 2015/08/26 at 23:08:50, bashi wrote: > > On 2015/08/26 11:50:23, nednguyen wrote: > > > > https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... > > > File tools/perf/measurements/renderer_memory.py (right): > > > > > > > https://codereview.chromium.org/1266833004/diff/140001/tools/perf/measurement... > > > tools/perf/measurements/renderer_memory.py:12: class > > > RendererMemory(page_test.PageTest): > > > I am not ok with adding another page_test. Please use timline based > measurement > > > for your metric need. You can ask perezju@ for how. > > > > Could you elaborate why you are not ok with adding a page test? Actually, we > don't want to have timeline based measurement. We want to have a snapshot of > memory dump at an arbitrary point. That's why I use tab.browser.DumpMemory(). > Could you suggest an alternative if this isn't acceptable? > > Requesting memory dumps at arbitrary points is supported by timeline based > measurements. > The memory_health_story.py I linked to, in fact, uses tab.browser.DumpMemory() > to request dumps. The reason we don't want to add another page test is because: 1) It's more code to review, more code to maintain. 2) You lose the extra features that's already support by timeline based measurement. In this patch, you didn't add the collected trace to results object. TBM does that so that it will later upload the trace to cloud storage & surface it on the dashboard for later view. 3) TimelineBasedMeasurement should make it more flexible for you to do what you want, not less. With your implementation, the act of requesting memory dump is fixated in the page test, whereas TBM enforces you do in in the page.
Patchset #9 (id:160001) has been deleted
Thanks for the clarification. Shifted to timeline based measurement. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:4: from telemetry.page import page as page_module On 2015/08/26 09:33:51, petrcermak wrote: > nit: Please put a blank line above this one. Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:15: url=url, page_set=page_set, name=name, On 2015/08/26 09:33:50, petrcermak wrote: > Lines 15-17 should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:18: self.arcive_data_file = 'data/blink_memory_mobile.json' On 2015/08/26 10:03:25, Sami wrote: > typo: archive_data_file Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:24: class TheVergePage(BlinkMemoryMobilePage): On 2015/08/26 10:03:26, Sami wrote: > Since this isn't enabled yet could we leave it out of this patch? I would like to keep this so that I can keep the TODO line. I'm worrying about record_wpr never get fixed. Also, I'd like to use this with --use-live-sites option. How about commenting this out? https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:27: # pylint: disable=C0301 On 2015/08/26 09:33:50, petrcermak wrote: > lines 27-30 should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:28: url='http://www.theverge.com/2015/8/11/9133883/taylor-swift-spotify-discover-weekly-what-is-going-on', On 2015/08/26 10:03:25, Sami wrote: > Looks like this line is too long. Tip: in python you can split strings across > lines like this: > > long_string = ('foo' > 'bar') Hmm, I don't think splitting URLs is a good idea. It would be difficult to read and copy-and-paste to omnibox. Other page sets don't split URLs. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:33: comment_link_selector = '.show_comments_link' On 2015/08/26 09:33:50, petrcermak wrote: > This should probably be a class constant Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:36: 'window.location.hash = "comments"') On 2015/08/26 09:33:50, petrcermak wrote: > This line should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:39: 'window.Chorus.Comments.collection.length > 0') On 2015/08/26 09:33:50, petrcermak wrote: > This line should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:45: url='https://mail.google.com/mail/', On 2015/08/26 09:33:50, petrcermak wrote: > Lines 45-47 should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:53: action_runner.WaitForNavigate() On 2015/08/26 10:03:26, Sami wrote: > RunNavigateSteps already waits for the navigation to complete so there's no need > to do it again here. Gmail login page redirects multiple times so I needed to add this. Maybe there is a way to handle such situation in a better way? https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:56: 'document.querySelector("#apploadingdiv").style.opacity == "0"') On 2015/08/26 09:33:50, petrcermak wrote: > This line should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:64: archive_data_file='data/blink_memory_mobile.json', On 2015/08/26 09:33:50, petrcermak wrote: > Lines 64-65 should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:67: # Why: High rate of Blink's memory consumption rate. On 2015/08/26 09:33:50, petrcermak wrote: > I don't understand what's the difference between "High rate of Blink's memory > consumption rate" and "Renderer memory usage is high". Is the former just more > specific? Why is it not "Blink memory usage is high" (consistency)? There are some components in renderer process; blink, content, gpu, v8, and so on. In the former, blink's memory usage is relativity high compared with other components, and in the latter, renderer memory usage itself is high, according to the memory-infra. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:69: url='https://www.pinterest.com', On 2015/08/26 09:33:50, petrcermak wrote: > Lines 69-71 should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:78: url='http://en.m.wikipedia.org/wiki/Wikipedia', On 2015/08/26 09:33:51, petrcermak wrote: > ditto (spaces) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:82: url='http://www.reddit.com/r/programming/comments/1g96ve', On 2015/08/26 09:33:50, petrcermak wrote: > ditto (spaces) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:86: # pylint: disable=line-too-long On 2015/08/26 09:33:50, petrcermak wrote: > ditto (spaces) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:86: # pylint: disable=line-too-long On 2015/08/26 10:03:25, Sami wrote: > On 2015/08/26 09:33:50, petrcermak wrote: > > ditto (spaces) > > Also please break the long line instead of turning off the linter. I prefer not splitting as described above. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:93: url='http://worldjournal.com/', On 2015/08/26 09:33:50, petrcermak wrote: > ditto (spaces) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:101: url='http://googlewebmastercentral.blogspot.com/2015/04/rolling-out-mobile-friendly-update.html?m=1', On 2015/08/26 09:33:50, petrcermak wrote: > ditto (spaces) Done. https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:105: # pylint: disable=line-too-long On 2015/08/26 09:33:50, petrcermak wrote: > ditto (spaces) Done.
https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:28: url='http://www.theverge.com/2015/8/11/9133883/taylor-swift-spotify-discover-weekly-what-is-going-on', On 2015/08/28 01:49:08, bashi1 wrote: > On 2015/08/26 10:03:25, Sami wrote: > > Looks like this line is too long. Tip: in python you can split strings across > > lines like this: > > > > long_string = ('foo' > > 'bar') > > Hmm, I don't think splitting URLs is a good idea. It would be difficult to read > and copy-and-paste to omnibox. Other page sets don't split URLs. I think Bashi's point is valid. You can wait for this to land: https://codereview.chromium.org/1306083005/
https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/140001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:24: class TheVergePage(BlinkMemoryMobilePage): On 2015/08/28 01:49:07, bashi1 wrote: > On 2015/08/26 10:03:26, Sami wrote: > > Since this isn't enabled yet could we leave it out of this patch? > > I would like to keep this so that I can keep the TODO line. I'm worrying about > record_wpr never get fixed. Also, I'd like to use this with --use-live-sites > option. How about commenting this out? I see. Keeping this here for the TODO sounds fine. https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:37: # pylint: disable=line-too-long Looks like the linter rule update landed already so you can leave these out now. https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:78: action_runner.WaitForNavigate() Could you add a comment here about the multiple redirects?
https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... File tools/perf/benchmarks/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... tools/perf/benchmarks/renderer_memory.py:25: options.AppendExtraBrowserArgs('--enable-memory-benchmarking') I worry a bit about duplicating these temporary workarounds on all memory benchmarks we might create in the future. Not sure what Ned thinks about this, but how about creating a perf_benchmark.MemoryBenchmark class where these tweaks can live, and then subclass from it on both memory_health_plan.py and here? https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... tools/perf/benchmarks/renderer_memory.py:31: return timeline_based_measurement.Options(overhead_level=trace_memory) Same as above, maybe move to a perf_benchmark.MemroyBenchmark parent class. https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:28: action_runner.ScrollPage() Not for this CL, but after running this benchmark for a while on the bots, and if you see a lot of noise in the measurements, you might want to add a short delay or some garbage collection before requesting the dump. Inspecting the generated traces can also give useful clues as to the best way to take these measurements.
On 2015/08/28 12:54:32, perezju wrote: > https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... > File tools/perf/benchmarks/renderer_memory.py (right): > > https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... > tools/perf/benchmarks/renderer_memory.py:25: > options.AppendExtraBrowserArgs('--enable-memory-benchmarking') > I worry a bit about duplicating these temporary workarounds on all memory > benchmarks we might create in the future. Not sure what Ned thinks about this, > but how about creating a perf_benchmark.MemoryBenchmark class where these tweaks > can live, and then subclass from it on both memory_health_plan.py and here? > > https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... > tools/perf/benchmarks/renderer_memory.py:31: return > timeline_based_measurement.Options(overhead_level=trace_memory) > Same as above, maybe move to a perf_benchmark.MemroyBenchmark parent class. > > https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... > File tools/perf/page_sets/blink_memory_mobile.py (right): > > https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... > tools/perf/page_sets/blink_memory_mobile.py:28: action_runner.ScrollPage() > Not for this CL, but after running this benchmark for a while on the bots, and > if you see a lot of noise in the measurements, you might want to add a short > delay or some garbage collection before requesting the dump. Inspecting the > generated traces can also give useful clues as to the best way to take these > measurements. Fyi:https://codereview.chromium.org/1313693006/
Thanks for reviews and sorry for the delay. https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... File tools/perf/benchmarks/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... tools/perf/benchmarks/renderer_memory.py:25: options.AppendExtraBrowserArgs('--enable-memory-benchmarking') On 2015/08/28 12:54:32, perezju wrote: > I worry a bit about duplicating these temporary workarounds on all memory > benchmarks we might create in the future. Not sure what Ned thinks about this, > but how about creating a perf_benchmark.MemoryBenchmark class where these tweaks > can live, and then subclass from it on both memory_health_plan.py and here? Sounds a good idea. Done. Ned, what do you think? https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... tools/perf/benchmarks/renderer_memory.py:31: return timeline_based_measurement.Options(overhead_level=trace_memory) On 2015/08/28 12:54:32, perezju wrote: > Same as above, maybe move to a perf_benchmark.MemroyBenchmark parent class. Done. https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:28: action_runner.ScrollPage() On 2015/08/28 12:54:32, perezju wrote: > Not for this CL, but after running this benchmark for a while on the bots, and > if you see a lot of noise in the measurements, you might want to add a short > delay or some garbage collection before requesting the dump. Inspecting the > generated traces can also give useful clues as to the best way to take these > measurements. Thanks for the suggestion. Added gc and delays (like you did in memory_health_story) https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:37: # pylint: disable=line-too-long On 2015/08/28 11:32:22, Sami wrote: > Looks like the linter rule update landed already so you can leave these out now. Done. https://codereview.chromium.org/1266833004/diff/180001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:78: action_runner.WaitForNavigate() On 2015/08/28 11:32:22, Sami wrote: > Could you add a comment here about the multiple redirects? Done.
https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... File tools/perf/benchmarks/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... tools/perf/benchmarks/renderer_memory.py:25: options.AppendExtraBrowserArgs('--enable-memory-benchmarking') On 2015/08/31 23:47:28, bashi1 wrote: > On 2015/08/28 12:54:32, perezju wrote: > > I worry a bit about duplicating these temporary workarounds on all memory > > benchmarks we might create in the future. Not sure what Ned thinks about this, > > but how about creating a perf_benchmark.MemoryBenchmark class where these caveat: the naming should be perf_benchmark._MemoryBenchmark so run_benchmark doesn't discover it. > tweaks > > can live, and then subclass from it on both memory_health_plan.py and here? > > Sounds a good idea. Done. Ned, what do you think? SGTM. We do this in many places in perf: https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/benchma...
https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... File tools/perf/benchmarks/renderer_memory.py (right): https://codereview.chromium.org/1266833004/diff/180001/tools/perf/benchmarks/... tools/perf/benchmarks/renderer_memory.py:25: options.AppendExtraBrowserArgs('--enable-memory-benchmarking') On 2015/09/01 17:52:38, nednguyen wrote: > On 2015/08/31 23:47:28, bashi1 wrote: > > On 2015/08/28 12:54:32, perezju wrote: > > > I worry a bit about duplicating these temporary workarounds on all memory > > > benchmarks we might create in the future. Not sure what Ned thinks about > this, > > > but how about creating a perf_benchmark.MemoryBenchmark class where these > caveat: the naming should be perf_benchmark._MemoryBenchmark so run_benchmark > doesn't discover it. Done. But I got following lint warning. "Access to a protected member _MemoryBenchmark of a client class (protected-access)" Just ignore the warning? > > tweaks > > > can live, and then subclass from it on both memory_health_plan.py and here? > > > > Sounds a good idea. Done. Ned, what do you think? > > SGTM. We do this in many places in perf: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/benchma...
https://codereview.chromium.org/1266833004/diff/220001/tools/perf/core/perf_b... File tools/perf/core/perf_benchmark.py (right): https://codereview.chromium.org/1266833004/diff/220001/tools/perf/core/perf_b... tools/perf/core/perf_benchmark.py:65: class _MemoryBenchmark(PerfBenchmark): Err, I suggest you to put _MemoryBenchmark & RendererMemoryBlinkMemoryMobile & MemoryHealthPlan benchmark in a same file.
Patchset #12 (id:240001) has been deleted
https://codereview.chromium.org/1266833004/diff/220001/tools/perf/core/perf_b... File tools/perf/core/perf_benchmark.py (right): https://codereview.chromium.org/1266833004/diff/220001/tools/perf/core/perf_b... tools/perf/core/perf_benchmark.py:65: class _MemoryBenchmark(PerfBenchmark): On 2015/09/01 22:40:17, nednguyen wrote: > Err, I suggest you to put _MemoryBenchmark & RendererMemoryBlinkMemoryMobile & > MemoryHealthPlan benchmark in a same file. Ah, I see. Done. Renamed memory_health_plan.py to memory_benchmark.py.
(non-owner) lgtm w/nits https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... File tools/perf/benchmarks/memory_benchmark.py (right): https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:17: """Base class for timeline based memory benchmark.""" nit: benchmark -> benchmarks https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:46: return bool(MemoryHealthPlan._RE_BENCHMARK_VALUES.match(value.name)) nit: MemoryHealthPlan -> cls https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:49: @benchmark.Enabled('android') Is there anything android-specific about this benchmark? Would it be useful to have it on all platforms? https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:65: RendererMemoryBlinkMemoryMobile._RE_RENDERER_VALUES.match(value.name)) nit: RendererMemoryBlinkMemoryMobile -> cls https://codereview.chromium.org/1266833004/diff/260001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/260001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:111: name='Wikipedia (1 tab) - delayed scroll start',)) nit: remove extra comma
lg2m with Juan's comments but let Ned have a look too.
https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... File tools/perf/benchmarks/memory_benchmark.py (right): https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:49: @benchmark.Enabled('android') On 2015/09/02 09:30:45, perezju wrote: > Is there anything android-specific about this benchmark? Would it be useful to > have it on all platforms? The fact that it uses BlinkMemoryMobilePageSet tell me that it shouldn't be used on other platform. But instead of using Enabled('android'), you should use SharedMobilePageState & leave out the Enabled(..). https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/page_se...
On 2015/09/02 16:30:07, nednguyen wrote: > https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... > File tools/perf/benchmarks/memory_benchmark.py (right): > > https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... > tools/perf/benchmarks/memory_benchmark.py:49: @benchmark.Enabled('android') > On 2015/09/02 09:30:45, perezju wrote: > > Is there anything android-specific about this benchmark? Would it be useful to > > have it on all platforms? > > The fact that it uses BlinkMemoryMobilePageSet tell me that it shouldn't be used > on other platform. > > But instead of using Enabled('android'), you should use SharedMobilePageState & > leave out the Enabled(..). > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/page_se... The rest are lg2me
https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... File tools/perf/benchmarks/memory_benchmark.py (right): https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:17: """Base class for timeline based memory benchmark.""" On 2015/09/02 09:30:45, perezju wrote: > nit: benchmark -> benchmarks Done. https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:46: return bool(MemoryHealthPlan._RE_BENCHMARK_VALUES.match(value.name)) On 2015/09/02 09:30:45, perezju wrote: > nit: MemoryHealthPlan -> cls Done. https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:49: @benchmark.Enabled('android') On 2015/09/02 16:30:07, nednguyen wrote: > On 2015/09/02 09:30:45, perezju wrote: > > Is there anything android-specific about this benchmark? Would it be useful to > > have it on all platforms? > > The fact that it uses BlinkMemoryMobilePageSet tell me that it shouldn't be used > on other platform. > > But instead of using Enabled('android'), you should use SharedMobilePageState & > leave out the Enabled(..). > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/page_se... BlinkMemoryMobilePage already uses ShareMobilePageState. Just removed Enabled(...). https://codereview.chromium.org/1266833004/diff/260001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:65: RendererMemoryBlinkMemoryMobile._RE_RENDERER_VALUES.match(value.name)) On 2015/09/02 09:30:45, perezju wrote: > nit: RendererMemoryBlinkMemoryMobile -> cls Done.
lgtm https://codereview.chromium.org/1266833004/diff/280001/tools/perf/benchmarks/... File tools/perf/benchmarks/memory_benchmark.py (right): https://codereview.chromium.org/1266833004/diff/280001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:32: @benchmark.Enabled('android') Removed?
https://codereview.chromium.org/1266833004/diff/280001/tools/perf/benchmarks/... File tools/perf/benchmarks/memory_benchmark.py (right): https://codereview.chromium.org/1266833004/diff/280001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:32: @benchmark.Enabled('android') On 2015/09/04 02:01:28, nednguyen wrote: > Removed? Forgot to remove this.. Done.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1266833004/#ps300001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266833004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266833004/300001
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...)
https://codereview.chromium.org/1266833004/diff/300001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/300001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:43: url='http://www.theverge.com/2015/8/11/9133883/taylor-swift-spotify-discover-weekly-what-is-going-on', To prevent lint error, move url= part up or remove it entirely. I only suppress long line check for lines with only the urls string. https://codereview.chromium.org/1266833004/diff/300001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:117: url='https://en.blog.wordpress.com/2012/09/04/freshly-pressed-editors-picks-for-august-2012/', ditto https://codereview.chromium.org/1266833004/diff/300001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:130: url='http://googlewebmastercentral.blogspot.com/2015/04/rolling-out-mobile-friendly-update.html?m=1', ditto
Thanks for the catch! https://codereview.chromium.org/1266833004/diff/300001/tools/perf/benchmarks/... File tools/perf/benchmarks/memory_benchmark.py (right): https://codereview.chromium.org/1266833004/diff/300001/tools/perf/benchmarks/... tools/perf/benchmarks/memory_benchmark.py:9: from telemetry import benchmark pylint also detected this unused import. https://codereview.chromium.org/1266833004/diff/300001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/300001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:43: url='http://www.theverge.com/2015/8/11/9133883/taylor-swift-spotify-discover-weekly-what-is-going-on', On 2015/09/04 02:52:12, nednguyen wrote: > To prevent lint error, move url= part up or remove it entirely. I only suppress > long line check for lines with only the urls string. Done. https://codereview.chromium.org/1266833004/diff/300001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:117: url='https://en.blog.wordpress.com/2012/09/04/freshly-pressed-editors-picks-for-august-2012/', On 2015/09/04 02:52:12, nednguyen wrote: > ditto Done. https://codereview.chromium.org/1266833004/diff/300001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:130: url='http://googlewebmastercentral.blogspot.com/2015/04/rolling-out-mobile-friendly-update.html?m=1', On 2015/09/04 02:52:12, nednguyen wrote: > ditto Done.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1266833004/#ps320001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266833004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266833004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1266833004/diff/320001/tools/perf/page_sets/b... File tools/perf/page_sets/blink_memory_mobile.py (right): https://codereview.chromium.org/1266833004/diff/320001/tools/perf/page_sets/b... tools/perf/page_sets/blink_memory_mobile.py:95: archive_data_file='data/blink_memory_mobile.json', You forgot to check in data/blink_memory_mobile.json?
On 2015/09/04 04:18:19, nednguyen wrote: > https://codereview.chromium.org/1266833004/diff/320001/tools/perf/page_sets/b... > File tools/perf/page_sets/blink_memory_mobile.py (right): > > https://codereview.chromium.org/1266833004/diff/320001/tools/perf/page_sets/b... > tools/perf/page_sets/blink_memory_mobile.py:95: > archive_data_file='data/blink_memory_mobile.json', > You forgot to check in data/blink_memory_mobile.json? Oh, I didn't know that blink_memory_mobile.json is required. Added.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1266833004/#ps340001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266833004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266833004/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2015/09/04 05:24:20, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) It seems that I uploaded the record to the wrong bucket (chromium-telemetry). Uploaded to chrome-partner-telemetry.
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266833004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266833004/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/09/04 06:07:54, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) It seems that no mmap dump found in a trace. primiano@, do you think we should add --no-sandbox option to options.AppendExtraBrowserArgs()? From failure log: Traceback (most recent call last): RunBenchmark at tools/telemetry/telemetry/internal/story_runner.py:289 Run(pt, stories, finder_options, results, benchmark.max_failures) Run at tools/telemetry/telemetry/internal/story_runner.py:217 _RunStoryAndProcessErrorIfNeeded(story, results, state, test) _RunStoryAndProcessErrorIfNeeded at tools/telemetry/telemetry/internal/story_runner.py:82 state.RunStory(results) RunStory at tools/telemetry/telemetry/page/shared_page_state.py:289 self._current_page, self._current_tab, results) ValidateAndMeasurePage at tools/telemetry/telemetry/web_perf/timeline_based_page_test.py:25 self._measurement.MeasureForPageTest(tracing_controller, results) MeasureForPageTest at tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:281 model = model_module.TimelineModel(trace_result) __init__ at tools/telemetry/telemetry/timeline/model.py:74 self.ImportTraces(trace_data, shift_world_to_zero=shift_world_to_zero) ImportTraces at tools/telemetry/telemetry/timeline/model.py:163 self.FinalizeImport(shift_world_to_zero, importers) FinalizeImport at tools/telemetry/telemetry/timeline/model.py:175 importer.FinalizeImport() FinalizeImport at tools/telemetry/telemetry/timeline/trace_event_importer.py:244 self._CreateMemoryDumps() _CreateMemoryDumps at tools/telemetry/telemetry/timeline/trace_event_importer.py:413 for events in self._all_memory_dumps_by_dump_id.itervalues()) SetGlobalMemoryDumps at tools/telemetry/telemetry/timeline/model.py:81 key=lambda dump: dump.start)) <genexpr> at tools/telemetry/telemetry/timeline/trace_event_importer.py:413 for events in self._all_memory_dumps_by_dump_id.itervalues()) __init__ at tools/telemetry/telemetry/timeline/memory_dump_event.py:273 assert len(have_mmaps) == 1 AssertionError
> primiano@, do you think we should add --no-sandbox option to > options.AppendExtraBrowserArgs()? Oh yes! you need that one to get mmaps on the renderer. Still didn't manage to get bandwidth to fix crbug.com/461788
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1266833004/#ps360001 (title: "Pass no-sandbox")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266833004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266833004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266833004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266833004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/09/08 01:40:33, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) From stdout: Traceback (most recent call last): File "/b/build/slave/android/build/src/tools/telemetry/telemetry/decorators.py", line 118, in wrapper func(*args, **kwargs) File "/b/build/slave/android/build/src/tools/perf/benchmarks/benchmark_smoke_unittest.py", line 73, in BenchmarkSmokeTest msg='Failed: %s' % benchmark) AssertionError: Failed: <class 'benchmarks.memory_benchmark.RendererMemoryBlinkMemoryMobile'> BenchmarkSmokeTest seems flaky on my device. I ran following command several times. $ tools/perf/run_tests --retry-limit 3 --device=xxx --browser=android-chromium benchmarks.benchmark_smoke_unittest.BenchmarkSmokeTest.memory.blink_memory_mobile First 2-3 attempts were timed out, but while I tried to find the cause using pdb, it started to pass for some reason. Do you guys have any ideas for the failure?
On 2015/09/08 06:34:05, bashi1 wrote: > On 2015/09/08 01:40:33, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > From stdout: > > Traceback (most recent call last): > File > "/b/build/slave/android/build/src/tools/telemetry/telemetry/decorators.py", line > 118, in wrapper > func(*args, **kwargs) > File > "/b/build/slave/android/build/src/tools/perf/benchmarks/benchmark_smoke_unittest.py", > line 73, in BenchmarkSmokeTest > msg='Failed: %s' % benchmark) > AssertionError: Failed: <class > 'benchmarks.memory_benchmark.RendererMemoryBlinkMemoryMobile'> > > BenchmarkSmokeTest seems flaky on my device. I ran following command several > times. > > $ tools/perf/run_tests --retry-limit 3 --device=xxx --browser=android-chromium > benchmarks.benchmark_smoke_unittest.BenchmarkSmokeTest.memory.blink_memory_mobile > > First 2-3 attempts were timed out, but while I tried to find the cause using > pdb, it started to pass for some reason. Do you guys have any ideas for the > failure? Hmm it looks like the device is timing out. Maybe the trybot devices are borked? What happens if when you run it locally? Similar timeouts? Have you checked in logcat if the browser crashes?
On 2015/09/08 08:02:25, Primiano Tucci wrote: > On 2015/09/08 06:34:05, bashi1 wrote: > > On 2015/09/08 01:40:33, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > From stdout: > > > > Traceback (most recent call last): > > File > > "/b/build/slave/android/build/src/tools/telemetry/telemetry/decorators.py", > line > > 118, in wrapper > > func(*args, **kwargs) > > File > > > "/b/build/slave/android/build/src/tools/perf/benchmarks/benchmark_smoke_unittest.py", > > line 73, in BenchmarkSmokeTest > > msg='Failed: %s' % benchmark) > > AssertionError: Failed: <class > > 'benchmarks.memory_benchmark.RendererMemoryBlinkMemoryMobile'> > > > > BenchmarkSmokeTest seems flaky on my device. I ran following command several > > times. > > > > $ tools/perf/run_tests --retry-limit 3 --device=xxx --browser=android-chromium > > > benchmarks.benchmark_smoke_unittest.BenchmarkSmokeTest.memory.blink_memory_mobile > > > > First 2-3 attempts were timed out, but while I tried to find the cause using > > pdb, it started to pass for some reason. Do you guys have any ideas for the > > failure? > > Hmm it looks like the device is timing out. Maybe the trybot devices are borked? > What happens if when you run it locally? Similar timeouts? Have you checked in > logcat if the browser crashes? Thanks Primiano for the reply. On my device, for first 2-3 attempts, nothing happened (even the browser didn't launch). I had to kill tests. But after I added a breakpoint in tools/telemetry/third_party/typ/typ/runner.py, it started passing. The test no longer fails on my device even when I removed the breakpoint. I haven't examined logcat.
On 2015/09/08 08:24:26, bashi1 wrote: > On 2015/09/08 08:02:25, Primiano Tucci wrote: > > On 2015/09/08 06:34:05, bashi1 wrote: > > > On 2015/09/08 01:40:33, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > > > From stdout: > > > > > > Traceback (most recent call last): > > > File > > > "/b/build/slave/android/build/src/tools/telemetry/telemetry/decorators.py", > > line > > > 118, in wrapper > > > func(*args, **kwargs) > > > File > > > > > > "/b/build/slave/android/build/src/tools/perf/benchmarks/benchmark_smoke_unittest.py", > > > line 73, in BenchmarkSmokeTest > > > msg='Failed: %s' % benchmark) > > > AssertionError: Failed: <class > > > 'benchmarks.memory_benchmark.RendererMemoryBlinkMemoryMobile'> > > > > > > BenchmarkSmokeTest seems flaky on my device. I ran following command several > > > times. > > > > > > $ tools/perf/run_tests --retry-limit 3 --device=xxx > --browser=android-chromium > > > > > > benchmarks.benchmark_smoke_unittest.BenchmarkSmokeTest.memory.blink_memory_mobile > > > > > > First 2-3 attempts were timed out, but while I tried to find the cause using > > > pdb, it started to pass for some reason. Do you guys have any ideas for the > > > failure? > > > > Hmm it looks like the device is timing out. Maybe the trybot devices are > borked? > > What happens if when you run it locally? Similar timeouts? Have you checked in > > logcat if the browser crashes? > > Thanks Primiano for the reply. On my device, for first 2-3 attempts, nothing > happened (even the browser didn't launch). I had to kill tests. But after I > added a breakpoint in tools/telemetry/third_party/typ/typ/runner.py, it started > passing. The test no longer fails on my device even when I removed the > breakpoint. I haven't examined logcat. I would suggest you to add @Disable('android') # crbug.com/... for now to land this patch & work on fixing the benchmark on android perf bot in a separate CL. That way allows us to make incremental progress, as this patch has been around for a while..
On 2015/09/08 16:59:07, nednguyen wrote: > On 2015/09/08 08:24:26, bashi1 wrote: > > On 2015/09/08 08:02:25, Primiano Tucci wrote: > > > On 2015/09/08 06:34:05, bashi1 wrote: > > > > On 2015/09/08 01:40:33, commit-bot: I haz the power wrote: > > > > > Try jobs failed on following builders: > > > > > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) > > > > > > > > From stdout: > > > > > > > > Traceback (most recent call last): > > > > File > > > > > "/b/build/slave/android/build/src/tools/telemetry/telemetry/decorators.py", > > > line > > > > 118, in wrapper > > > > func(*args, **kwargs) > > > > File > > > > > > > > > > "/b/build/slave/android/build/src/tools/perf/benchmarks/benchmark_smoke_unittest.py", > > > > line 73, in BenchmarkSmokeTest > > > > msg='Failed: %s' % benchmark) > > > > AssertionError: Failed: <class > > > > 'benchmarks.memory_benchmark.RendererMemoryBlinkMemoryMobile'> > > > > > > > > BenchmarkSmokeTest seems flaky on my device. I ran following command > several > > > > times. > > > > > > > > $ tools/perf/run_tests --retry-limit 3 --device=xxx > > --browser=android-chromium > > > > > > > > > > benchmarks.benchmark_smoke_unittest.BenchmarkSmokeTest.memory.blink_memory_mobile > > > > > > > > First 2-3 attempts were timed out, but while I tried to find the cause > using > > > > pdb, it started to pass for some reason. Do you guys have any ideas for > the > > > > failure? > > > > > > Hmm it looks like the device is timing out. Maybe the trybot devices are > > borked? > > > What happens if when you run it locally? Similar timeouts? Have you checked > in > > > logcat if the browser crashes? > > > > Thanks Primiano for the reply. On my device, for first 2-3 attempts, nothing > > happened (even the browser didn't launch). I had to kill tests. But after I > > added a breakpoint in tools/telemetry/third_party/typ/typ/runner.py, it > started > > passing. The test no longer fails on my device even when I removed the > > breakpoint. I haven't examined logcat. > > I would suggest you to add @Disable('android') # crbug.com/... for now to land > this patch & work on fixing the benchmark on android perf bot in a separate CL. > That way allows us to make incremental progress, as this patch has been around > for a while.. Err, nvm what I said. It looks like that these benchmarks only run on mobile, so there is not much point in landing the code if it doesn't run on mobile.
> Err, nvm what I said. It looks like that these benchmarks only run on mobile, so > there is not much point in landing the code if it doesn't run on mobile. Is there any differences between smoke tests and run_benchmark? I can't reproduce the failure on my machine and I have no idea the cause. What can I do to land this CL?
On 2015/09/09 05:59:32, bashi1 wrote: > > Err, nvm what I said. It looks like that these benchmarks only run on mobile, > so > > there is not much point in landing the code if it doesn't run on mobile. > > Is there any differences between smoke tests and run_benchmark? I can't > reproduce the failure on my machine and I have no idea the cause. What can I do > to land this CL? Hmm wait what happened to the --enable-memory-benchmarking flag? I think it got lost on the road That is required to disable the periodic tracing behavior (workaround for crbug.com/529184), otherwise your trace will get spammed with too much dump points. Probably the trace spam causes the trace to be too big and the download timeout.
Please ignore my comment in #88, I just cannot read python inheritance at 9AM in the morning. (I keep telling myself that I should refrain to write any email before drinking coffee in the morning) The flag is there, it's all good. Can't tell why this is timing out though :/ maybe related with what jbudorick was trying to fix in crrev.com/1328093006 (which got reverted) that was causing deadlocks in telemetry if the browser crashes?
On 2015/09/09 08:51:51, Primiano Tucci wrote: > Please ignore my comment in #88, I just cannot read python inheritance at 9AM in > the morning. > (I keep telling myself that I should refrain to write any email before drinking > coffee in the morning) > > The flag is there, it's all good. > Can't tell why this is timing out though :/ > > maybe related with what jbudorick was trying to fix in crrev.com/1328093006 > (which got reverted) that was causing deadlocks in telemetry if the browser > crashes? np, thanks for the help (as usual)! Probably wait for re-land of the CL (it seems reverted) and retry to commit?
On 2015/09/09 08:58:09, bashi1 wrote: > On 2015/09/09 08:51:51, Primiano Tucci wrote: > > Please ignore my comment in #88, I just cannot read python inheritance at 9AM > in > > the morning. > > (I keep telling myself that I should refrain to write any email before > drinking > > coffee in the morning) > > > > The flag is there, it's all good. > > Can't tell why this is timing out though :/ > > > > maybe related with what jbudorick was trying to fix in crrev.com/1328093006 > > (which got reverted) that was causing deadlocks in telemetry if the browser > > crashes? > > np, thanks for the help (as usual)! > > Probably wait for re-land of the CL (it seems reverted) and retry to commit? You can also kick off tryjob on nexus bots with "tools/perf/run_benchmark ...--browser=trybot-all-android". If they all green, feel free to disable the benchmark in https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/benchma..., land this & file a bug to reenable the smoke test.
On 2015/09/09 15:17:48, nednguyen wrote: > On 2015/09/09 08:58:09, bashi1 wrote: > > On 2015/09/09 08:51:51, Primiano Tucci wrote: > > > Please ignore my comment in #88, I just cannot read python inheritance at > 9AM > > in > > > the morning. > > > (I keep telling myself that I should refrain to write any email before > > drinking > > > coffee in the morning) > > > > > > The flag is there, it's all good. > > > Can't tell why this is timing out though :/ > > > > > > maybe related with what jbudorick was trying to fix in crrev.com/1328093006 > > > (which got reverted) that was causing deadlocks in telemetry if the browser > > > crashes? > > > > np, thanks for the help (as usual)! > > > > Probably wait for re-land of the CL (it seems reverted) and retry to commit? > > You can also kick off tryjob on nexus bots with "tools/perf/run_benchmark > ...--browser=trybot-all-android". If they all green, feel free to disable the > benchmark in > https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/benchma..., > land this & file a bug to reenable the smoke test. Thanks for the info. I tried to run tryjobs, but it seems that bots are unhappy with my CL :( Some bots failed to compile (without CL), and others complained that there is no memory.blink_memory_mobile benchmark, which I defined in this CL. Sample output: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_one_perf... CL for tryjobs: https://codereview.chromium.org/1334123002/ What am I missing? Are these bots reliable?
On 2015/09/11 06:02:05, bashi1 wrote: > On 2015/09/09 15:17:48, nednguyen wrote: > > On 2015/09/09 08:58:09, bashi1 wrote: > > > On 2015/09/09 08:51:51, Primiano Tucci wrote: > > > > Please ignore my comment in #88, I just cannot read python inheritance at > > 9AM > > > in > > > > the morning. > > > > (I keep telling myself that I should refrain to write any email before > > > drinking > > > > coffee in the morning) > > > > > > > > The flag is there, it's all good. > > > > Can't tell why this is timing out though :/ > > > > > > > > maybe related with what jbudorick was trying to fix in > crrev.com/1328093006 > > > > (which got reverted) that was causing deadlocks in telemetry if the > browser > > > > crashes? > > > > > > np, thanks for the help (as usual)! > > > > > > Probably wait for re-land of the CL (it seems reverted) and retry to commit? > > > > You can also kick off tryjob on nexus bots with "tools/perf/run_benchmark > > ...--browser=trybot-all-android". If they all green, feel free to disable the > > benchmark in > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/benchma..., > > land this & file a bug to reenable the smoke test. > > Thanks for the info. I tried to run tryjobs, but it seems that bots are unhappy > with my CL :( Some bots failed to compile (without CL), and others complained > that there is no memory.blink_memory_mobile benchmark, which I defined in this > CL. Sample output: > > http://build.chromium.org/p/tryserver.chromium.perf/builders/android_one_perf... > > CL for tryjobs: > https://codereview.chromium.org/1334123002/ > > What am I missing? Are these bots reliable? http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_p... The "(with patch)" one succeeded with numbers outpu: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_p.... The without patch is a little bit dumb as it doesn't know that the benchmark wasn't defined without your patch. You can ignore the compile errors one & go ahead disable the smoke test & land this :-)
> The without patch is a little bit dumb as it doesn't know that the benchmark > wasn't defined without your patch. > > You can ignore the compile errors one & go ahead disable the smoke test & land > this :-) I see. Disabled the test for now. Hope it works :)
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1266833004/#ps380001 (title: "Disabled")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266833004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266833004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_perf_bisect on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266833004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266833004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_perf_bisect on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
On 2015/09/15 07:37:50, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_perf_bisect on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL) No clue:(
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266833004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266833004/380001
On 2015/09/15 08:50:32, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1266833004/380001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1266833004/380001 that feels like an infra failure. At this point I'd NOTRY=true
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_perf_bisect on tryserver.chromium.perf (JOB_FAILED, no build URL)
On 2015/09/15 08:51:45, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_perf_bisect on tryserver.chromium.perf (JOB_FAILED, no build URL) Removed mac_perf_bisect. Trying again.
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266833004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266833004/380001
Message was sent while issue was closed.
Committed patchset #18 (id:380001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/0836ebab5ec66a9092722acacdbacc15b02b18ce Cr-Commit-Position: refs/heads/master@{#348864}
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:380001) has been created in https://codereview.chromium.org/1347663003/ by zhenw@chromium.org. The reason for reverting is: memory.memory_health_plan failure on chromium.perf Linux/Mac/Win bots See https://code.google.com/p/chromium/issues/detail?id=532075.
Message was sent while issue was closed.
Thanks for the revert.
It seems that we do need the @benchmark.Enabled('android'), at least on
memory.memory_health_plan.
Message was sent while issue was closed.
On 2015/09/15 16:06:39, perezju wrote:
> Thanks for the revert.
>
> It seems that we do need the @benchmark.Enabled('android'), at least on
> memory.memory_health_plan.
This is strange. Feel free to reland this benchmark with that decorator & file a
bug to track this issue.
Message was sent while issue was closed.
On 2015/09/15 17:11:57, nednguyen wrote:
> On 2015/09/15 16:06:39, perezju wrote:
> > Thanks for the revert.
> >
> > It seems that we do need the @benchmark.Enabled('android'), at least on
> > memory.memory_health_plan.
>
> This is strange. Feel free to reland this benchmark with that decorator & file
a
> bug to track this issue.
Thanks for the revert and suggestions. I'll add the decorator and try to re-land
in a separate issue.
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/0836ebab5ec66a9092722acacdbacc15b02b18ce Cr-Commit-Position: refs/heads/master@{#348864} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
