Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1427163003:
[Discardable Shared Memory] Adding Windows support. (Closed)
Description[Discardable Shared Memory] Adding Windows support.
1) Purge(): Now doing VirtualFree MEM_DECOMMIT back to XP during a purge.
This releases the physical storage, either in memory or in the paging file on disk.
Pages remain RESERVED. NOTE: this is the best that can be done immediately
from the browser, until the child/renderer notices the purge and releases its own
handles.
2) Lock/Unlock() pinning: For >= Win8, we have access to MEM_RESET_UNDO support.
On Unlock(), we now MEM_RESET. On Lock(), we MEM_RESET_UNDO.
This mimics the existing Ashmem support on Android.
(See bug ticket for why OfferVirtualMemory/ReclaimVirtualMemory is not being used.)
BUG=180334
R=reveman@chromium.org,jschuh@chromium.org
Committed: https://crrev.com/498c30ec10c16ba90cd425836c5a92ac0d685c36
Cr-Commit-Position: refs/heads/master@{#358466}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Review fixes part 1. #
Total comments: 2
Patch Set 3 : Review fixes part 2. #
Total comments: 16
Patch Set 4 : Review fixes part 3. (Static Windows version back (but cleaner), and more comments.) #
Total comments: 2
Patch Set 5 : Review fixes part 4. (One CL to rule them all?) #Messages
Total messages: 27 (7 generated)
cool! maybe you already did but if not please run "cl format" on this patch https://codereview.chromium.org/1427163003/diff/1/base/memory/discardable_sha... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:128: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { base::win::GetVersion() is cheap and already cached in a singleton so no need for another level of caching here. Can we just move this check to the Lock() function and remove this DoPinning function? https://codereview.chromium.org/1427163003/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:253: if (DoPinning()) { this check is never false on android so unnecessary in that case.. https://codereview.chromium.org/1427163003/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:263: if (nullptr == VirtualAlloc(reinterpret_cast<char*>( nit: chromium style is "if (!VirtualAlloc(reinte..." https://codereview.chromium.org/1427163003/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:302: if (nullptr == VirtualAlloc(reinterpret_cast<char*>( if (!VirtualAlloc(reinte...
New upload incoming. Thanks for your review David, I appreciate it. https://codereview.chromium.org/1427163003/diff/1/base/memory/discardable_sha... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:128: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { On 2015/11/03 00:23:57, reveman wrote: > base::win::GetVersion() is cheap and already cached in a singleton so no need > for another level of caching here. Can we just move this check to the Lock() > function and remove this DoPinning function? Thank for this! I missed the fact that OSInfo acts as a singleton and caches it for me. This is way cleaner now. https://codereview.chromium.org/1427163003/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:253: if (DoPinning()) { On 2015/11/03 00:23:57, reveman wrote: > this check is never false on android so unnecessary in that case.. Done. https://codereview.chromium.org/1427163003/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:263: if (nullptr == VirtualAlloc(reinterpret_cast<char*>( On 2015/11/03 00:23:57, reveman wrote: > nit: chromium style is "if (!VirtualAlloc(reinte..." Done. https://codereview.chromium.org/1427163003/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:302: if (nullptr == VirtualAlloc(reinterpret_cast<char*>( On 2015/11/03 00:23:57, reveman wrote: > if (!VirtualAlloc(reinte... Done.
lgtm with nit but please have someone more familiar with the VirtualAlloc windows API review it before it lands. https://codereview.chromium.org/1427163003/diff/20001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/20001/base/memory/discardable... base/memory/discardable_shared_memory.cc:369: // child/renderer notices the purge and releases its own handles. nit: concepts such as browser and renderer process should not leak into base/. if we need this comment here then maybe "purging process" instead of browser and "client process" instead of child/renderer.
https://codereview.chromium.org/1427163003/diff/20001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/20001/base/memory/discardable... base/memory/discardable_shared_memory.cc:369: // child/renderer notices the purge and releases its own handles. On 2015/11/03 01:42:31, reveman wrote: > nit: concepts such as browser and renderer process should not leak into base/. > if we need this comment here then maybe "purging process" instead of browser and > "client process" instead of child/renderer. Done.
https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:231: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { Save the version off into a static, otherwise you're traversing the singleton pointers on every call. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:266: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { This shouldn't be version gated. It's supported on all Windows versions as far as we're concerned. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:273: } Looks like the indentation is wrong and it should be inside the #if block. I'm guessing this is why the compile is failing. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:368: if (!VirtualFree(reinterpret_cast<char*>(shared_memory_.memory()) + This doesn't look like what you want -- it's basically equivalent to POSIX mprotect(PROT_NONE). The correct approach would be some combination of the following: * DiscardVirtualMemory() - fast, but isn't guaranteed to zero-fill, requires Win8+, and is flaky on Win10, so you still have to fallback to one of the following * MEM_DECOMMIT followed by MEM_COMMIT - Two syscalls, so a bit slower but will guarantee zero-fill * MEM_RESET - One syscall, but releases the memory lazily and doesn't guarantee zero-fill
Thanks for the Windows API usage checks Justin! See notes. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:231: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { On 2015/11/03 05:11:24, jschuh (very slow) wrote: > Save the version off into a static, otherwise you're traversing the singleton > pointers on every call. In upload #1 I was doing a lazy init of a static variable for this. However, reveman thought that since OSInfo acts as a singleton and caches its own OS version, it would fast and cleaner to just access that. Further thoughts? https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:266: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { On 2015/11/03 05:11:24, jschuh (very slow) wrote: > This shouldn't be version gated. It's supported on all Windows versions as far > as we're concerned. So this is in the Unlock function, and needs to match the Lock function above for pinning. Since MEM_RESET_UNDO is only supported on >= Win8 for the Lock, we can only do the RESET on >= Win8 as well. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:273: } On 2015/11/03 05:11:24, jschuh (very slow) wrote: > Looks like the indentation is wrong and it should be inside the #if block. I'm > guessing this is why the compile is failing. Done. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:368: if (!VirtualFree(reinterpret_cast<char*>(shared_memory_.memory()) + On 2015/11/03 05:11:24, jschuh (very slow) wrote: > This doesn't look like what you want -- it's basically equivalent to POSIX > mprotect(PROT_NONE). The correct approach would be some combination of the > following: > * DiscardVirtualMemory() - fast, but isn't guaranteed to zero-fill, > requires Win8+, and is flaky on Win10, so you still have to fallback > to one of the following > * MEM_DECOMMIT followed by MEM_COMMIT - Two syscalls, so a bit slower but > will guarantee zero-fill > * MEM_RESET - One syscall, but releases the memory lazily and doesn't > guarantee zero-fill So a few notes: - We don't need to guarantee any sort of zero fill on purge for the functioning of DSM. - This memory won't be used again, it'll just be asynchronously freed "later". - This is the Purge function, so we're trying to do the most releasing of resources that we can here (this function is called synchronously from the browser process as soon as there is a resource problem). We can't do a full free at this point, until the child/renderer process notices that there has been a purge and it can release its own references. This is my understanding from what reveman has told me, but I'm happy to get clarification from him on that. - I was originally going to just MEM_RESET here, but given the above point, it's not clear why I wouldn't just DECOMMIT to do an immediate release of resource. Also, on >= win8, an unlocked segment that would be a candidate for purge will already be in MEM_RESET state (from the last Unlock). That's not a deal-breaker or anything, but it would be nice to do an immediate release on purge.
Description was changed from ========== [Discardable Shared Memory] Adding Windows support. 1) Purge(): Now doing VirtualFree MEM_DECOMMIT back to XP during a purge. This releases the physical storage, either in memory or in the paging file on disk. Pages remain RESERVED. NOTE: this is the best that can be done immediately from the browser, until the child/renderer notices the purge and releases its own handles. 2) Lock/Unlock() pinning: For >= Win8, we have access to MEM_RESET_UNDO support. On Unlock(), we now MEM_RESET. On Lock(), we MEM_RESET_UNDO. This mimics the existing Ashmem support on Android. (See bug ticket for why OfferVirtualMemory/ReclaimVirtualMemory is not being used.) BUG=180334 R=reveman@chromium.org,jschuh@chromium.org ========== to ========== [Discardable Shared Memory] Adding Windows support. 1) Purge(): Now doing VirtualFree MEM_DECOMMIT back to XP during a purge. This releases the physical storage, either in memory or in the paging file on disk. Pages remain RESERVED. NOTE: this is the best that can be done immediately from the browser, until the child/renderer notices the purge and releases its own handles. 2) Lock/Unlock() pinning: For >= Win8, we have access to MEM_RESET_UNDO support. On Unlock(), we now MEM_RESET. On Lock(), we MEM_RESET_UNDO. This mimics the existing Ashmem support on Android. (See bug ticket for why OfferVirtualMemory/ReclaimVirtualMemory is not being used.) BUG=180334 R=reveman@chromium.org,jschuh@chromium.org ==========
pennymac@chromium.org changed reviewers: + brucedawson@chromium.org
Memory is so darned messy. I wrote a couple of short novels rambling about the questions you asked. I didn't examine the rest of the code in any detail. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:231: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { Both options are quite cheap, with the exact cost depending on whether LTCG (buildtype=Official only) is in a good mood. Calling GetVersion() each time requires (in the generated code) testing a pointer and then dereferencing it, with possible some function calls in there. Adding a static requires testing a flag and then returning the cached value. Calling GetVersion() is more likely to touch more cache lines and make actual function calls, whereas a static means introducing a couple more globals. If LTCG aggressively inlines GetInstance() then this makes the static option wasteful since that code will rarely be used. It's practically a wash and I doubt this is called enough to matter, but I'd lean towards the static. Examining the machine code to see what actually happens in official builds is not worth it. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:368: if (!VirtualFree(reinterpret_cast<char*>(shared_memory_.memory()) + MEM_DECOMMIT will indeed free up the pages, making them fully available to other processes or the file cache. This seems like absolutely the right thing to do when we are under memory pressure. MEM_RESET seems appropriate for if we want the OS to decide whether or not it needs the memory, and that doesn't sound like the case here. I don't understand this: > This memory won't be used again, it'll just be asynchronously > freed "later". I understand the first part, but not the second. MEM_DECOMMIT will free the pages immediately. FWIW, the cost of doing a MEM_DECOMMIT and then a MEM_COMMIT of the same address range is *not* the cost of the two system calls - those are very cheap. The cost is the time to remove the pages from the memory map, zero them, and then fault them back in. So, we should only MEM_DECOMMIT memory that we don't think we'll need for a bit. Tricky, I know. Now, the cost of MEM_RESET where the memory is trimmed from the working set and then brought back in later should be comparable. However if we are wrong and the pages are reused by Chrome shortly afterwards then MEM_RESET will be faster because the memory mapping doesn't get changed. I analyze and measured these hidden costs last year and blogged about them: https://randomascii.wordpress.com/2014/12/10/hidden-costs-of-memory-allocation/ Actual timing results depend significantly on your CPU and OS, I think.
Okay, thanks for all the context, sorry for the confusion, and lgtm. My only nits are that the comments I mention below would be very helpful Independent of this CL, I think "Pin and Unpin" would have been a much clearer API naming choice than "Lock and Unlock". However, that's a totally separate discussion. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:231: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { On 2015/11/03 18:25:44, penny wrote: > On 2015/11/03 05:11:24, jschuh (very slow) wrote: > > Save the version off into a static, otherwise you're traversing the singleton > > pointers on every call. > > In upload #1 I was doing a lazy init of a static variable for this. However, > reveman thought that since OSInfo acts as a singleton and caches its own OS > version, it would fast and cleaner to just access that. > > Further thoughts? Honestly, I prefer the other but I don't feel strongly either way and can't say authoritatively which is better. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:266: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { On 2015/11/03 18:25:44, penny wrote: > On 2015/11/03 05:11:24, jschuh (very slow) wrote: > > This shouldn't be version gated. It's supported on all Windows versions as far > > as we're concerned. > > So this is in the Unlock function, and needs to match the Lock function above > for pinning. Since MEM_RESET_UNDO is only supported on >= Win8 for the Lock, we > can only do the RESET on >= Win8 as well. Fair enough, this should be biased to preserving the contents of memory. Could we get a comment somewhere to that effect, because in reading the code I assumed it should be biased to releasing memory. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:368: if (!VirtualFree(reinterpret_cast<char*>(shared_memory_.memory()) + On 2015/11/03 18:25:44, penny wrote: > On 2015/11/03 05:11:24, jschuh (very slow) wrote: > > This doesn't look like what you want -- it's basically equivalent to POSIX > > mprotect(PROT_NONE). The correct approach would be some combination of the > > following: > > * DiscardVirtualMemory() - fast, but isn't guaranteed to zero-fill, > > requires Win8+, and is flaky on Win10, so you still have to fallback > > to one of the following > > * MEM_DECOMMIT followed by MEM_COMMIT - Two syscalls, so a bit slower but > > will guarantee zero-fill > > * MEM_RESET - One syscall, but releases the memory lazily and doesn't > > guarantee zero-fill > > So a few notes: > - We don't need to guarantee any sort of zero fill on purge for the functioning > of DSM. > - This memory won't be used again, it'll just be asynchronously freed "later". > - This is the Purge function, so we're trying to do the most releasing of > resources that we can here (this function is called synchronously from the > browser process as soon as there is a resource problem). We can't do a full > free at this point, until the child/renderer process notices that there has been > a purge and it can release its own references. This is my understanding from > what reveman has told me, but I'm happy to get clarification from him on that. > - I was originally going to just MEM_RESET here, but given the above point, it's > not clear why I wouldn't just DECOMMIT to do an immediate release of resource. > Also, on >= win8, an unlocked segment that would be a candidate for purge will > already be in MEM_RESET state (from the last Unlock). That's not a deal-breaker > or anything, but it would be nice to do an immediate release on purge. Okay, please tweak the comment above on the OS_POSIX section so it clearly explains that we're not using this address range anymore, but using madvise() this way because it's faster.
Thanks Bruce and Justin for input. David, let me know if you're happy with my new implementation of a static variable for the Windows version. It doesn't impact non-Windows anymore. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:231: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { On 2015/11/04 21:52:43, jschuh (very slow) wrote: > On 2015/11/03 18:25:44, penny wrote: > > On 2015/11/03 05:11:24, jschuh (very slow) wrote: > > > Save the version off into a static, otherwise you're traversing the > singleton > > > pointers on every call. > > > > In upload #1 I was doing a lazy init of a static variable for this. However, > > reveman thought that since OSInfo acts as a singleton and caches its own OS > > version, it would fast and cleaner to just access that. > > > > Further thoughts? > > Honestly, I prefer the other but I don't feel strongly either way and can't say > authoritatively which is better. Thanks Bruce and Justin for input here. I don't feel emotional about this either. I've gone back to a static class variable, but I've made things cleaner than my first upload. The lazy init happens inline in Lock or Unlock now, with now extra function call in there. David, let me know if you're happier with this new version as well! https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:266: if (base::win::GetVersion() >= base::win::VERSION_WIN8) { On 2015/11/04 21:52:43, jschuh (very slow) wrote: > On 2015/11/03 18:25:44, penny wrote: > > On 2015/11/03 05:11:24, jschuh (very slow) wrote: > > > This shouldn't be version gated. It's supported on all Windows versions as > far > > > as we're concerned. > > > > So this is in the Unlock function, and needs to match the Lock function above > > for pinning. Since MEM_RESET_UNDO is only supported on >= Win8 for the Lock, > we > > can only do the RESET on >= Win8 as well. > > Fair enough, this should be biased to preserving the contents of memory. Could > we get a comment somewhere to that effect, because in reading the code I assumed > it should be biased to releasing memory. I've updated the comments to try to provide clarity. https://codereview.chromium.org/1427163003/diff/40001/base/memory/discardable... base/memory/discardable_shared_memory.cc:368: if (!VirtualFree(reinterpret_cast<char*>(shared_memory_.memory()) + On 2015/11/04 21:52:43, jschuh (very slow) wrote: > On 2015/11/03 18:25:44, penny wrote: > > On 2015/11/03 05:11:24, jschuh (very slow) wrote: > > > This doesn't look like what you want -- it's basically equivalent to POSIX > > > mprotect(PROT_NONE). The correct approach would be some combination of the > > > following: > > > * DiscardVirtualMemory() - fast, but isn't guaranteed to zero-fill, > > > requires Win8+, and is flaky on Win10, so you still have to fallback > > > to one of the following > > > * MEM_DECOMMIT followed by MEM_COMMIT - Two syscalls, so a bit slower but > > > will guarantee zero-fill > > > * MEM_RESET - One syscall, but releases the memory lazily and doesn't > > > guarantee zero-fill > > > > So a few notes: > > - We don't need to guarantee any sort of zero fill on purge for the > functioning > > of DSM. > > - This memory won't be used again, it'll just be asynchronously freed "later". > > - This is the Purge function, so we're trying to do the most releasing of > > resources that we can here (this function is called synchronously from the > > browser process as soon as there is a resource problem). We can't do a full > > free at this point, until the child/renderer process notices that there has > been > > a purge and it can release its own references. This is my understanding from > > what reveman has told me, but I'm happy to get clarification from him on that. > > > - I was originally going to just MEM_RESET here, but given the above point, > it's > > not clear why I wouldn't just DECOMMIT to do an immediate release of resource. > > > Also, on >= win8, an unlocked segment that would be a candidate for purge will > > already be in MEM_RESET state (from the last Unlock). That's not a > deal-breaker > > or anything, but it would be nice to do an immediate release on purge. > > Okay, please tweak the comment above on the OS_POSIX section so it clearly > explains that we're not using this address range anymore, but using madvise() > this way because it's faster. I've added comments for this section, providing more context - without mentioning "browser" vs "renderer" in base\.
Sorry, not lgtm. Do we have data showing that this version value needs to be cached? If we do, why is it not done in windows_version.cc in a way that all callers would benefit from? https://codereview.chromium.org/1427163003/diff/60001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/60001/base/memory/discardable... base/memory/discardable_shared_memory.cc:230: ((osVersion = base::win::GetVersion()) >= base::win::VERSION_WIN8))) { I'm not a fan of this. It's hard to read. There's a race condition as this is called on multiple threads [1] and the hard-to-read logic is duplicated below. [1] it's a subtle race condition that won't cause harm but we avoid this in chromium afaik. we should be using LazyInstance if we need to cache this value.
https://codereview.chromium.org/1427163003/diff/60001/base/memory/discardable... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1427163003/diff/60001/base/memory/discardable... base/memory/discardable_shared_memory.cc:230: ((osVersion = base::win::GetVersion()) >= base::win::VERSION_WIN8))) { On 2015/11/05 19:13:47, reveman wrote: > I'm not a fan of this. It's hard to read. There's a race condition as this is > called on multiple threads [1] and the hard-to-read logic is duplicated below. > > [1] it's a subtle race condition that won't cause harm but we avoid this in > chromium afaik. we should be using LazyInstance if we need to cache this value. Bruce, Justin and I don't feel strongly about this, so I've reverted to just referencing the OSInfo version every call to Lock/Unlock. Both implementations will be quite close in efficiency after optimizations.
On 2015/11/05 19:51:29, penny wrote: > https://codereview.chromium.org/1427163003/diff/60001/base/memory/discardable... > File base/memory/discardable_shared_memory.cc (right): > > https://codereview.chromium.org/1427163003/diff/60001/base/memory/discardable... > base/memory/discardable_shared_memory.cc:230: ((osVersion = > base::win::GetVersion()) >= base::win::VERSION_WIN8))) { > On 2015/11/05 19:13:47, reveman wrote: > > I'm not a fan of this. It's hard to read. There's a race condition as this is > > called on multiple threads [1] and the hard-to-read logic is duplicated below. > > > > [1] it's a subtle race condition that won't cause harm but we avoid this in > > chromium afaik. we should be using LazyInstance if we need to cache this > value. > > Bruce, Justin and I don't feel strongly about this, so I've reverted to just > referencing the OSInfo version every call to Lock/Unlock. Both implementations > will be quite close in efficiency after optimizations. Can I get a final lgtm on the change back David?
lgtm
The CQ bit was checked by pennymac@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jschuh@chromium.org Link to the patchset: https://codereview.chromium.org/1427163003/#ps80001 (title: "Review fixes part 4. (One CL to rule them all?)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427163003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427163003/80001
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...)
pennymac@chromium.org changed reviewers: + danakj@chromium.org, thestig@chromium.org
stampy lgtm
The CQ bit was checked by pennymac@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427163003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427163003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/498c30ec10c16ba90cd425836c5a92ac0d685c36 Cr-Commit-Position: refs/heads/master@{#358466}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1473733005/ by nednguyen@google.com. The reason for reverting is: Speculative revert: this may cause regression to smoothness.tough_canvas_cases BUG=555225. |
