Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11667030:
Add GPU memory usage contents browser test (Closed)
|
Created:
7 years, 12 months ago by ccameron Modified:
7 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, kkania, jam, apatrick_chromium, chrome-speed-team+watch_google.com, robertshield, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a pass/fail test to verify that GPU memory usage doesn't grow beyond an acceptable level when using CSS (managed memory) and WebGL (unmanaged memory).
BUG=135525
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175753
Patch Set 1 #
Total comments: 1
Patch Set 2 : Further clean up tests #Patch Set 3 : More cleanup #Patch Set 4 : Parameterize memory use through JS call #Patch Set 5 : Remove extraneous comments #
Total comments: 16
Patch Set 6 : Change to be a content browser test #Patch Set 7 : Contents browser test ready for review #
Total comments: 16
Patch Set 8 : Incorporate review feedback #Patch Set 9 : Remove unused file #
Total comments: 8
Patch Set 10 : Incorporate review feedback #
Total comments: 16
Patch Set 11 : Incorporate review feedback #Patch Set 12 : Correct limit call #Patch Set 13 : Remove controversial functionality #
Messages
Total messages: 28 (0 generated)
https://codereview.chromium.org/11667030/diff/1/chrome/test/perf/gpu_memory_t... File chrome/test/perf/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/1/chrome/test/perf/gpu_memory_t... chrome/test/perf/gpu_memory_test.cc:59: if (!automation()->GetBrowserWindow(0)->SendJSONRequest( To get memory information from the test, I through JSON -- there aren't too many instances of this being done, but there aren't too many instances of other things being done. Is this the right thing to do?
I added some comments in areas that I'm structurally unsure of. If those are sane, then this is ready for a thorough review. https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... File chrome/test/gpu/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:19: #include "gpu/command_buffer/service/gpu_switches.h" Is this a layering violation -- should the switch be in public? https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:86: std::string request, response, error; I use JSON to get the memory values. There is not much precedent for this for non-Python tests, but there seemed less precedence for adding things to the automation proxy. Is this a reasonable scheme? https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:165: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(2000)); Given how big the pages are (256MB for some), waiting 2 seconds seemed like a reasonable time -- we need to wait not only for the first present, but we need to wait for the new memory policy to come to renderer, be enforced, and then committed (so it's not like one clear thing to wait for). Is there a better way to do this? https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:177: TEST_F(GpuMemoryTest, Simple) { These tests will explode if the GPU process is disabled -- should I test for this or do we assume that the GPU exists in the GPU tests.
https://codereview.chromium.org/11667030/diff/8002/chrome/test/data/gpu/mem_c... File chrome/test/data/gpu/mem_css3d.html (right): https://codereview.chromium.org/11667030/diff/8002/chrome/test/data/gpu/mem_c... chrome/test/data/gpu/mem_css3d.html:33: block.style.WebkitTransform = "rotateX(-" + i + "deg)"; Does this actually put every one of these elements into its own composited layer? https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... File chrome/test/gpu/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:135: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(200)); Sleep() seems like a potential source of flakiness. What do other tests do? Is there an onload() message that you could be waiting for, instead?
https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... File chrome/test/gpu/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:30: class GpuMemoryTest : public UITest { Does this truly needs to be a UI test? Will content browsertests be enough? If yes, then you can directly query memory usage and other states. https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:56: const FilePath kCurrentDir(FilePath::kCurrentDirectory); kCurrentDir is never used. https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:86: std::string request, response, error; On 2013/01/03 19:07:56, ccameron1 wrote: > I use JSON to get the memory values. There is not much precedent for this for > non-Python tests, but there seemed less precedence for adding things to the > automation proxy. Is this a reasonable scheme? I don't see any problem doing it, but I am not an expert in precedence in chrome. https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:165: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(2000)); On 2013/01/03 19:07:56, ccameron1 wrote: > Given how big the pages are (256MB for some), waiting 2 seconds seemed like a > reasonable time -- we need to wait not only for the first present, but we need > to wait for the new memory policy to come to renderer, be enforced, and then > committed (so it's not like one clear thing to wait for). Is there a better way > to do this? I truly think this is bad practice. It wastes time on fast bots, and cause potential flakiness in slow bots in debug mode. You should find a better way of testing this - add new communication mechanisms if necessary. https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:177: TEST_F(GpuMemoryTest, Simple) { On 2013/01/03 19:07:56, ccameron1 wrote: > These tests will explode if the GPU process is disabled -- should I test for > this or do we assume that the GPU exists in the GPU tests. You can use trace events to make sure GPU process is there. See gpu_feature_browsertest.cc, RunEventTest for example. But it's up to you. I think doing this adds some flakiness to the test, due to some unknown flakiness in the tracing mechanism.
Can you make this a browser test? Better yet, can it be a content browser test?
I've converted this to be a browser test. It still has some issues, so I've re-added the not-ready-for-review tag. https://codereview.chromium.org/11667030/diff/8002/chrome/test/data/gpu/mem_c... File chrome/test/data/gpu/mem_css3d.html (right): https://codereview.chromium.org/11667030/diff/8002/chrome/test/data/gpu/mem_c... chrome/test/data/gpu/mem_css3d.html:33: block.style.WebkitTransform = "rotateX(-" + i + "deg)"; Yes. I verify that the page uses about the expected amount of memory (to make sure the tests that we're staying under budget are meaningful). I've also updated the transform to not look as bad (fwiw). https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... File chrome/test/gpu/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:30: class GpuMemoryTest : public UITest { Yes, this should have been content browser tests. https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:56: const FilePath kCurrentDir(FilePath::kCurrentDirectory); Removed (from the new test). https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:86: std::string request, response, error; Changing to browser test removes this. https://codereview.chromium.org/11667030/diff/8002/chrome/test/gpu/gpu_memory... chrome/test/gpu/gpu_memory_test.cc:135: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(200)); This will be a lot of work, but I've wrapped my head around it. We need to flush the following things 1. flush all pending commits on Renderers' main threads to the impl thread (causing allocations) 2. flush all allocations that the new tree will cause from the impl thread to the gpu process 3. flush any memory management policy changes from the gpu process to the impl thread of all renderer processes 4. flush any "commit now required" messages caused by changing texture policy on the impl thread back to the main thread This forms a cycle of 1->2->3->4->1. It's not so much a "flushing" as inserting a token into the message loop and waiting for it to pass from one place to another (sometimes waiting for something happen before moving on, e.g, waiting on the impl thread until all painting is done). We need to wait for this cycle to run its course a few times (say, 5-10), then we can be sure that any effects we wanted to observe should have happened. This is similar to jbauman's latency measurement system, but we'll need to determine if any of it can be done. Because that is a really big change, I'd like to check in this change (when it's ready) and just have the test not report failure (so we can make sure that it is stable, and so that it doesn't pollute the other, only-semi-related change).
This should be ready for review now. I used a somewhat weak condition to determine if a certain amount of GPU memory is in use -- I just continually sample GPU memory use until it remains in the expected range for a while. This could allow things to pass that should fail. I outline a more rigorous solution in comments. I also moved the gpu bot test switch to a different path because this is the first-non-chrome test to use the flag.
not fully done reading the patch but rough suggestion is a few smaller patches since this broadly looks good but this is important enough to get some really good scrutiny on every single step. https://codereview.chromium.org/11667030/diff/16001/content/browser/gpu/gpu_m... File content/browser/gpu/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/16001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:215: IN_PROC_BROWSER_TEST_F(GpuMemoryTest, Simple) { Here and elsewhere the test name should say imply what is being verified. E.g. VerifyThatSingleTabStaysWithinMemoryLimit. Or something like that. Think from the PoV of a random dude seeing your test name failing, along with the ~10 commit messages trying to figure out if its flake or some legit issue. https://codereview.chromium.org/11667030/diff/16001/content/common/gpu/gpu_me... File content/common/gpu/gpu_memory_manager.cc (left): https://codereview.chromium.org/11667030/diff/16001/content/common/gpu/gpu_me... content/common/gpu/gpu_memory_manager.cc:424: video_memory_usage_stats->process_map[ as an aside, any reason we say video memory rather than gpu memory? it feels a bit inconsistent https://codereview.chromium.org/11667030/diff/16001/content/public/common/con... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/11667030/diff/16001/content/public/common/con... content/public/common/content_switches.cc:151: // Use hardware gpu, if available, for tests. might want to pull this out to a separate cl https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... File content/test/data/gpu/mem_css3d.html (right): https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... content/test/data/gpu/mem_css3d.html:26: // Generate n 3D CSS elements that are each about 1 MB in size how about splitting this patch up into a few patches, where the one we focus on at first is just a single patch that verifies that this specific page consumes at least a certain amount of memory? That deals with a lot of the same issues but also shortens the patch a bit. https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... content/test/data/gpu/mem_css3d.html:34: block.style.WebkitTransform = "rotateZ(" + i + "deg) translate3d(" + i + "px, " + i + "px, " + 2*(i-n+1) + "px)"; is the rotation important? is there a simpler case we can come up with? I'm worrying about the hackability of this say, in 3 years, when we start like raytracing things or something crazy like that and suddenly this page isn't using 1mb in size... e.g. what about a bunch of semi 50% transparent layers all in the exact same place but with webkit-transform: translateZ(0) https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... content/test/data/gpu/mem_css3d.html:35: block.style.zIndex = -i; is the backward zIndex ordering important? https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... content/test/data/gpu/mem_css3d.html:40: block.innerHTML = i; prefer block.textContent = i; unless you're inserting actual html https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... File content/test/data/gpu/mem_webgl.html (right): https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... content/test/data/gpu/mem_webgl.html:133: <div class="test-div" style="width=100px; height=100px;"> is the outer test-div important here?
Thanks!! https://codereview.chromium.org/11667030/diff/16001/content/browser/gpu/gpu_m... File content/browser/gpu/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/16001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:215: IN_PROC_BROWSER_TEST_F(GpuMemoryTest, Simple) { On 2013/01/05 05:14:42, nduca wrote: > Here and elsewhere the test name should say imply what is being verified. Good point -- I've updated the test names to not spare characters. https://codereview.chromium.org/11667030/diff/16001/content/common/gpu/gpu_me... File content/common/gpu/gpu_memory_manager.cc (left): https://codereview.chromium.org/11667030/diff/16001/content/common/gpu/gpu_me... content/common/gpu/gpu_memory_manager.cc:424: video_memory_usage_stats->process_map[ On 2013/01/05 05:14:42, nduca wrote: > as an aside, any reason we say video memory rather than gpu memory? The changes adding video_memory came about before I'd converged on a name. I should do a re-naming change. https://codereview.chromium.org/11667030/diff/16001/content/public/common/con... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/11667030/diff/16001/content/public/common/con... content/public/common/content_switches.cc:151: // Use hardware gpu, if available, for tests. On 2013/01/05 05:14:42, nduca wrote: > might want to pull this out to a separate cl Good call, put it in https://codereview.chromium.org/11777019/ https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... File content/test/data/gpu/mem_css3d.html (right): https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... content/test/data/gpu/mem_css3d.html:26: // Generate n 3D CSS elements that are each about 1 MB in size On 2013/01/05 05:14:42, nduca wrote: > how about splitting this patch up into a few patches, where the one we focus on > at first is just a single patch that verifies that this specific page consumes > at least a certain amount of memory? That deals with a lot of the same issues > but also shortens the patch a bit. This I feel strongly about. I had considered doing that, but the complexity of parameterizing the amount of memory to consume in JS fairly small (~5 lines) in the test source, and even smaller in the HTML (~1 line). If I were to have separate patch, it would be this exact same HTML file, but with an onload="useGpuMemory(256)" added. I'd much rather follow this scheme of using HTML files that have a JS command to consume a certain amount of GPU memory than check in "HTML file to use X MB". https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... content/test/data/gpu/mem_css3d.html:34: block.style.WebkitTransform = "rotateZ(" + i + "deg) translate3d(" + i + "px, " + i + "px, " + 2*(i-n+1) + "px)"; On 2013/01/05 05:14:42, nduca wrote: > is the rotation important? > > is there a simpler case we can come up with? I'm worrying about the hackability > of this say, in 3 years, when we start like raytracing things or something crazy > like that and suddenly this page isn't using 1mb in size... Importantly, the test does verify that these pages use the amount of memory that we expect them to use, so if someone puts in a clever memory-saving patch, it will break this test with a message saying "hey, you've gotten too smart for this test case, please make this test case smarter, too". > e.g. what about a bunch of semi 50% transparent layers all in the exact same > place but with webkit-transform: translateZ(0) The rotation is to frustrate clever culling that the compositor does. I have emotional reasons to not like transparency, but transparency does make things simpler, so I've switched to using that. I did add a translateZ(i), because if they're all at the same position, they are degenerate and not-so-well-defined. https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... content/test/data/gpu/mem_css3d.html:35: block.style.zIndex = -i; On 2013/01/05 05:14:42, nduca wrote: > is the backward zIndex ordering important? Nope, that was a mistake -- I'd meant translate3d. Torched. https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... content/test/data/gpu/mem_css3d.html:40: block.innerHTML = i; On 2013/01/05 05:14:42, nduca wrote: > prefer > block.textContent = i; > > unless you're inserting actual html Fixed. https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... File content/test/data/gpu/mem_webgl.html (right): https://codereview.chromium.org/11667030/diff/16001/content/test/data/gpu/mem... content/test/data/gpu/mem_webgl.html:133: <div class="test-div" style="width=100px; height=100px;"> On 2013/01/05 05:14:42, nduca wrote: > is the outer test-div important here? Nope, annihilated.
LGTM. https://codereview.chromium.org/11667030/diff/20011/content/browser/gpu/gpu_m... File content/browser/gpu/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/20011/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:14: #include "content/public/browser/notification_types.h" Are these two notification* header needed? https://codereview.chromium.org/11667030/diff/20011/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:32: const char *kMemoryLimitSwitch = "256"; nit: style violation, * should be together with char https://codereview.chromium.org/11667030/diff/20011/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:183: void WaitForMemoryStableInRange(size_t low, size_t high) { nit: indentation wrong https://codereview.chromium.org/11667030/diff/20011/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:190: while (same_value_observed_count < 20) { This is ugly and could cause potential flakiness. at least you should add a TODO for future improvement.
Thanks!! https://codereview.chromium.org/11667030/diff/20011/content/browser/gpu/gpu_m... File content/browser/gpu/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/20011/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:14: #include "content/public/browser/notification_types.h" On 2013/01/07 22:56:07, Zhenyao Mo wrote: > Are these two notification* header needed? No -- removed. I went through the remaining #includes and removed the ones that this compiled without. https://codereview.chromium.org/11667030/diff/20011/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:32: const char *kMemoryLimitSwitch = "256"; On 2013/01/07 22:56:07, Zhenyao Mo wrote: > nit: style violation, * should be together with char Done. https://codereview.chromium.org/11667030/diff/20011/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:183: void WaitForMemoryStableInRange(size_t low, size_t high) { On 2013/01/07 22:56:07, Zhenyao Mo wrote: > nit: indentation wrong Done. https://codereview.chromium.org/11667030/diff/20011/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:190: while (same_value_observed_count < 20) { On 2013/01/07 22:56:07, Zhenyao Mo wrote: > This is ugly and could cause potential flakiness. at least you should add a > TODO for future improvement. Added a comment pointing to the TODO in ExpectMemoryUsageGE. It's important to note that the flakiness will only be of the sort of "this test passes when it should fail", never the other way around.
Adding piman for OWNER review of content/ Adding cdn for OWNER (security) review of gpu_messages.h
https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... File content/browser/gpu/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:24: // and extra 16MB of wiggle-room for over-allocation. nit: s/16/24/ given the value below https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:57: // Will break after OnVideoMemoryUsageStatsUpdate nit: not sure what this comment means. Consider removing, or adding details. https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:114: default: nit: just remove and have the compiler warn about missing cases. https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:135: return observer.GetBytesAllocated()/1048576; nit: spaces around / https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:154: WaitForMemoryStableInRange(mbytes_expected, static_cast<size_t>(-1)); nit: std::memory_limits<size_t>::max() https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:176: // Wait until we get the same vaue in the expected interval 20 times in typo: vaue->value https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:193: } This can't return failure, it will just not return instead if the conditions aren't met. It seems problematic for a test... Is there a way we can make the measurement more stable (e.g. wait for 5 rAF in javascript), and directly check the value? https://codereview.chromium.org/11667030/diff/24001/content/test/data/gpu/mem... File content/test/data/gpu/mem_css3d.html (right): https://codereview.chromium.org/11667030/diff/24001/content/test/data/gpu/mem... content/test/data/gpu/mem_css3d.html:40: Can we force layout here to ensure we don't depend on a non-rAF timer to allocate the memory? I forgot which one, but I think just reading a window property is enough (maybe ask James, I think he knows).
Thanks! Updated. We should discuss how to flush out all of the pending memory changes before checking the allocation value. The scheme used here came out of the discussion earlier on the review. https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... File content/browser/gpu/gpu_memory_test.cc (right): https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:24: // and extra 16MB of wiggle-room for over-allocation. On 2013/01/08 00:04:17, piman wrote: > nit: s/16/24/ given the value below Done. https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:57: // Will break after OnVideoMemoryUsageStatsUpdate On 2013/01/08 00:04:17, piman wrote: > nit: not sure what this comment means. Consider removing, or adding details. This was to say the Run() call will stop after the OnVideoMmemoryUsageStatsUpdate is called. I've removed the comment entirely, since it's pretty clear. https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:114: default: On 2013/01/08 00:04:17, piman wrote: > nit: just remove and have the compiler warn about missing cases. Done. https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:135: return observer.GetBytesAllocated()/1048576; On 2013/01/08 00:04:17, piman wrote: > nit: spaces around / Done. https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:154: WaitForMemoryStableInRange(mbytes_expected, static_cast<size_t>(-1)); On 2013/01/08 00:04:17, piman wrote: > nit: std::memory_limits<size_t>::max() Done. https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:176: // Wait until we get the same vaue in the expected interval 20 times in On 2013/01/08 00:04:17, piman wrote: > typo: vaue->value Done. https://codereview.chromium.org/11667030/diff/24001/content/browser/gpu/gpu_m... content/browser/gpu/gpu_memory_test.cc:193: } On 2013/01/08 00:04:17, piman wrote: > This can't return failure, it will just not return instead if the conditions > aren't met. It seems problematic for a test... Is there a way we can make the > measurement more stable (e.g. wait for 5 rAF in javascript), and directly check > the value? Yes, the failure method is time-out. The comments in ExpectMemoryUsageGE outline the necessary conditions for being able to verify that the result isn't read. Note that we need to be sure that the messages have reached backgrounded tabs, commits occur, GPU memory manage messages are sent, the messages are acted on, impl-side painting jobs finish (cause that's when memory is actually allocated), and so on. The plan is to add a mechanism for flushing all of these activities in a separate CL (or series of CLs). There is a patch that jbauman is working on which to do something similar at https://codereview.chromium.org/11293121/ LMK if I should drop by to discuss this (I'd like for there to be a simpler-but-still-robust mechanism). https://codereview.chromium.org/11667030/diff/24001/content/test/data/gpu/mem... File content/test/data/gpu/mem_css3d.html (right): https://codereview.chromium.org/11667030/diff/24001/content/test/data/gpu/mem... content/test/data/gpu/mem_css3d.html:40: On 2013/01/08 00:04:17, piman wrote: > Can we force layout here to ensure we don't depend on a non-rAF timer to > allocate the memory? I forgot which one, but I think just reading a window > property is enough (maybe ask James, I think he knows). James said the incantation is document.body.offsetTop; Added.
I discussed this a bit with piman, and we're going to set a lower timeout threshold. The default test timeout is 45 seconds, which would be catastrophic on the waterfall. We're going to do - wait for 5 RAFs on all visible windows - then wait at most N seconds (where N is near 1 or 5) for the GPU memory value to reach the desired state I'll run this by zmo and the other people who were concerned about flake (at present, we're at N=45 already).
A substantial portion of me looks at this waiting and raffing and says "look, this is a code smell. We should do that thing that you said was hard but correct."
On 2013/01/08 10:12:38, nduca wrote: > A substantial portion of me looks at this waiting and raffing and says "look, > this is a code smell. We should do that thing that you said was hard but > correct." The plan is to add the full flush loop, but there's already pressure to keep get this CL smaller, and the outlined flush loop touches a lot of files. Of note is that waiting for RAFs is actually part of the cycle mentioned (in particular, steps 1 and 2 listed in gpu_memory_test.cc). The remaining parts are covered by adding a Browser->Renderer->Gpu->(wait for manage if one is pending)->Renderer->Browser flush loop. (And the one other remaining part is making renderers wait until their impl-side painting jobs are all done, which can easily be added to the mentioned loop).
When you're faced with that sort of feedback, the correct thing to do is not do everything in one patch. You could land something that doesn't even include the basic test, but that has the shape of things. Then add more and more with smaller patches until a single test turns on.
On 2013/01/08 18:44:46, nduca wrote: > When you're faced with that sort of feedback, the correct thing to do is not do > everything in one patch. You could land something that doesn't even include the > basic test, but that has the shape of things. Then add more and more with > smaller patches until a single test turns on. What I think I'm hearing is that you'd rather this patch go in without actually enabling the tests (or perhaps even including them in the source). That's absolutely fine by me -- please confirm that that that's what you're saying, and I'll totally do it.
I've trimmed this down with the goal of getting the basics checked in, then iterating on the more controversial parts (RAFs, timeouts, counting, etc) in separate CLs This patch has no tests (so it's just dead code), and has no mechanism for waiting for GPU memory policy to shake out -- the "query GPU memory" function gets you what's there at this exact instant (this function will continue to exist). ptal (and thanks for the attention through the iterations)
LGTM as is
lgtmnomnomnomnom
IPC security audit LGTM
Thanks!!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/11667030/28001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/11667030/28001
Message was sent while issue was closed.
Change committed as 175753 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
