Significant, continuously degrading, performance regression from running AdBlock extension. |
|||||||||||||||
Issue descriptionSee attached trace, renderer titled "Traps and Mines" TileManager::ScheduleTasks has a wall duration of 5s! ChildSharedBitmapManager::AllocateSharedMemoryBitmap varies from 100ms to 500ms. The call is stuck in a sync IPC to the browser process. This is on a 64GB RAM machine, of which only ~25GB is in use. chrome version: 54.0.2831.0 Looking at the browser process, the IO thread is totally hosed. MessageLoop::RunTask is called non-stop, with about 35ms per run on average. I sampled the browser process and discovered that most of the time in the IOThread is spent in STL container operations. extensions::WebRequestEventDetails::OnDeterminedFrameData seems to be the relevant call. I turned off AdBlock (3.1.1), and the problem went away. Without delving too much further into this: 1) Can we move this call off the IOThread? Any call that might hose the IOThread should never be allowed. 2) If you look at the sample, the call to extensions::WebRequestEventDetails::OnDeterminedFrameData has hundreds (thousands?!) of sub-frames. Almost every single one is deallocating std::tree nodes. This suggests that there is an algorithm inefficiency. Given the magnitude of this performance issue, and the number of users who have AdBlock, I think this issue deserves Pri-1, and further investigation.
,
Sep 1 2016
,
Sep 1 2016
Doing more digging to see when these listeners get created: Wow! That's a lot of copies being made.
"""
+ ! : | + ! : | + ! : | + ! 621 ChromeNetworkDelegate::OnBeforeURLRequest(net::URLRequest*, base::Callback<void (int), (base::internal::CopyMode)1> const&, GURL*) (in Google Chrome Framework) load address 0x103667000 + 0x150b83f [chrome_network_delegate.cc:286]
+ ! : | + ! : | + ! : | + ! 621 extensions::ExtensionWebRequestEventRouter::OnBeforeRequest(void*, extensions::InfoMap const*, net::URLRequest*, base::Callback<void (int), (base::internal::CopyMode)1> const&, GURL*) (in Google Chrome Framework) load address 0x103667000 + 0x122c65a [web_request_api.cc:624]
+ ! : | + ! : | + ! : | + ! 439 extensions::ExtensionWebRequestEventRouter::DispatchEvent(void*, net::URLRequest*, std::__1::vector<extensions::ExtensionWebRequestEventRouter::EventListener const*, std::__1::allocator<extensions::ExtensionWebRequestEventRouter::EventListener const*> > const&, std::__1::unique_ptr<extensions::WebRequestEventDetails, std::__1::default_delete<extensions::WebRequestEventDetails> >) (in Google Chrome Framework) load address 0x103667000 + 0x122d2a6 [vector:1591]
+ ! : | + ! : | + ! : | + ! : 184 extensions::ExtensionWebRequestEventRouter::EventListener::EventListener(extensions::ExtensionWebRequestEventRouter::EventListener const&) (in Google Chrome Framework) load address 0x103667000 + 0x1235146 [__tree:142]
+ ! : | + ! : | + ! : | + ! : 105 extensions::ExtensionWebRequestEventRouter::EventListener::EventListener(extensions::ExtensionWebRequestEventRouter::EventListener const&) (in Google Chrome Framework) load address 0x103667000 + 0x12350e6 [memory:1774]
"""
"""
+ ! : | + ! : | 373 extensions::ExtensionWebRequestEventRouter::DispatchEvent(void*, net::URLRequest*, std::__1::vector<extensions::ExtensionWebRequestEventRouter::EventListener const*, std::__1::allocator<extensions::ExtensionWebRequestEventRouter::EventListener const*> > const&, std::__1::unique_ptr<extensions::WebRequestEventDetails, std::__1::default_delete<extensions::WebRequestEventDetails> >) (in Google Chrome Framework) load address 0x103667000 + 0x122d438 [callback_internal.h:109]
+ ! : | + ! : | 373 extensions::WebRequestEventDetails::DetermineFrameDataOnIO(base::Callback<void (std::__1::unique_ptr<extensions::WebRequestEventDetails, std::__1::default_delete<extensions::WebRequestEventDetails> >), (base::internal::CopyMode)1> const&) (in Google Chrome Framework) load address 0x103667000 + 0x123e27d [callback_internal.h:109]
...
+ ! : | + ! : | ! : 366 std::__1::__vector_base<extensions::ExtensionWebRequestEventRouter::EventListener, std::__1::allocator<extensions::ExtensionWebRequestEventRouter::EventListener> >::~__vector_base() (in Google Chrome Framework) load address 0x103667000 + 0x12346c8 [weak_ptr.h:246]
,
Sep 1 2016
I built Chromium, adding some logging around the size of blocked_requests, and installed AdBlock. The set grows without bounds! Nothing is ever removed from it. Combined with a bunch of copies of EventListener on every request, and you've got a crazy memory/performance regression that is guaranteed to hit every user of AdBlock (and presumably other, comparable extensions). I reverted """ Fix WebRequest's EventListener::operator< (Closed) https://codereview.chromium.org/2121873002 """ and logging shows that blocked_requests stays at size 1-2. So here are the problems that I see: 1) (Pri-0) blocked_requests grows without bounds. Combined with frequent copies of ExtensionWebRequestEventRouter::EventListener, we have a guaranteed, incremental, performance regression for a huge number of users. 2) Why are these so many copies of EventListener being made? I see one place where we make an explicit copy, and then a places where we pass by copy a vector of EventListeners, when we could be passing by reference. 3) Why is blocked_requests a mutable std::set? "mutable" is a huge red flag that something is going wrong, as this is a misuse of that keyword. I see no reason why we shouldn't use unordered_set.
,
Sep 1 2016
,
Sep 1 2016
I haven't confirmed the bug on Windows/Linux, but I see no reason why it wouldn't occur.
,
Sep 1 2016
Thanks for the investigation! 1) It sounds like our RemoveListener logic isn't working since https://codereview.chromium.org/2121873002 changed how we find matching listeners. 2), 3) No good reason for either, AFAICT. Let's fix it! :) Are you planning to take on the patch? :)
,
Sep 1 2016
yeah, looking into it now.
,
Sep 1 2016
Erik: looks like this might be a dupe of issue 641958 ?
,
Sep 1 2016
Yea, sounds like the same thing. I've CQ-ed a revert.
,
Sep 1 2016
Thanks - I'm working on a long term fix [note that the revert means that we'll start crashing again...].
,
Sep 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e579d519620e6583ddc304f22c4c3c71dabbbe20 commit e579d519620e6583ddc304f22c4c3c71dabbbe20 Author: mmenke <mmenke@chromium.org> Date: Thu Sep 01 22:41:14 2016 Revert of Fix WebRequest's EventListener::operator< (patchset #3 id:40001 of https://codereview.chromium.org/2121873002/ ) Reason for revert: This may have caused a perf regression. Reverting to verify. If it did not, I'll reland next week. BUG= 641958 , 643025 Original issue's description: > Fix WebRequest's EventListener::operator< > > std::set's count() method is expected to return 0 or 1. But due > to the fact that EventListener::operator< conditionally ignored > |embedder_process_id|, the count method could return more than 1 > (e.g. 2) in release builds on Windows (VC++), and fail an assertion. > > This assertion failure shows that the assumption about the uniqueness > of EventListeners (while ignoring the |embedder_process_id| value) does > not hold, i.e. there may be more than one non-webview EventListeners > with the same |extension_id| and |sub_event_name|, but different > |embedder_process_id|. > > To fix this, this patch removes the conditional exclusion of > |embedder_process_id| from operator< and looks up |embedder_process_id| > before searching through the set. > > BUG=589735 > TEST=./unit_tests --gtest_filter=ExtensionWebRequestTest.AddAndRemoveListeners > > Committed: https://crrev.com/c89943b168ef2ccc51f3ec0720e552f36ec31351 > Cr-Commit-Position: refs/heads/master@{#404456} TBR=rockot@chromium.org,rob@robwu.nl # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=589735 Review-Url: https://codereview.chromium.org/2299373002 Cr-Commit-Position: refs/heads/master@{#416094} [modify] https://crrev.com/e579d519620e6583ddc304f22c4c3c71dabbbe20/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc [modify] https://crrev.com/e579d519620e6583ddc304f22c4c3c71dabbbe20/extensions/browser/api/web_request/web_request_api.cc [modify] https://crrev.com/e579d519620e6583ddc304f22c4c3c71dabbbe20/extensions/browser/api/web_request/web_request_api.h
,
Sep 1 2016
Dropping the priority since the CL in question has been reverted. [Note that this causes slightly more crashes].
,
Sep 2 2016
This issue is marked as Release Block Dev. Please make sure to land the fix before 5:00 PM PST, Monday (09/05/16).
,
Sep 6 2016
Tagging as RBB. Erik, could you please whether the performance regression is fixed in canary? If so please request a merge to M54 ASAP.
,
Sep 6 2016
Let's merge the revert to M54, and let the crash fix + cleanup target M55.
,
Sep 6 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a67f30a6b303582d930417197f4a351901c6898a commit a67f30a6b303582d930417197f4a351901c6898a Author: Erik Chen <erikchen@chromium.org> Date: Tue Sep 06 23:56:27 2016 [Merge to 2840] Revert of Fix WebRequest's EventListener::operator< (patchset #3 id:40001 of https://codereview.chromium.org/2121873002/ ) > Reason for revert: > This may have caused a perf regression. Reverting to verify. If it did not, I'll reland next week. > > BUG= 641958 , 643025 > > Original issue's description: > > Fix WebRequest's EventListener::operator< > > > > std::set's count() method is expected to return 0 or 1. But due > > to the fact that EventListener::operator< conditionally ignored > > |embedder_process_id|, the count method could return more than 1 > > (e.g. 2) in release builds on Windows (VC++), and fail an assertion. > > > > This assertion failure shows that the assumption about the uniqueness > > of EventListeners (while ignoring the |embedder_process_id| value) does > > not hold, i.e. there may be more than one non-webview EventListeners > > with the same |extension_id| and |sub_event_name|, but different > > |embedder_process_id|. > > > > To fix this, this patch removes the conditional exclusion of > > |embedder_process_id| from operator< and looks up |embedder_process_id| > > before searching through the set. > > > > BUG=589735 > > TEST=./unit_tests --gtest_filter=ExtensionWebRequestTest.AddAndRemoveListeners > > > > Committed: https://crrev.com/c89943b168ef2ccc51f3ec0720e552f36ec31351 > > Cr-Commit-Position: refs/heads/master@{#404456} > > TBR=rockot@chromium.org,rob@robwu.nl > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=589735 > > Review-Url: https://codereview.chromium.org/2299373002 > Cr-Commit-Position: refs/heads/master@{#416094} > (cherry picked from commit e579d519620e6583ddc304f22c4c3c71dabbbe20) Review URL: https://codereview.chromium.org/2317923002 . Cr-Commit-Position: refs/branch-heads/2840@{#192} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/a67f30a6b303582d930417197f4a351901c6898a/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc [modify] https://crrev.com/a67f30a6b303582d930417197f4a351901c6898a/extensions/browser/api/web_request/web_request_api.cc [modify] https://crrev.com/a67f30a6b303582d930417197f4a351901c6898a/extensions/browser/api/web_request/web_request_api.h
,
Sep 7 2016
Thanks for the revert. As we are targeting a fix in M55, tagging accordingly.
,
Sep 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7cdfec136b896222345c88f56e89a2d40d53e49c commit 7cdfec136b896222345c88f56e89a2d40d53e49c Author: erikchen <erikchen@chromium.org> Date: Thu Sep 08 21:14:46 2016 Fix semantics of ExtensionWebRequestEventRouter::EventListeners. There are two types of EventListeners: those with web_view_instance_id and those without. EventListeners with a web_view_instance_id need to always consider embedder_process_id for equality comparisons. EventListeners without a web_view_instance_id should sometimes ignore embedder_process_id (when being removed) for equality comparisons. The previous code had two major issues: 1) EventListeners were stored in a std::set. std:set requires a stable sort operator. There is no way to create a stable sort operator that sometimes ignores embedder_process_id. Aside: std::set is almost never the right container to use from a performance perspective. In this case, the code frequently needs to iterate through the entire container away, so I've changed the container to be a std::vector (we expect the total number of elements to be small). If we find performance issues, we should switch to std::unordered_set. 2) EventListeners need to be messaged asynchronously, but it's possible that the EventListener is no longer registered by the time the callback comes around. The previous code would make a copy of the EventListener, and then try to "find" the original copy in the callback. Once again, this requires clear semantics for equality. The new code makes several changes: 1) Create the class EventListener::ID. As its name implies, this uniquely identifies an EventListener. 2) Disallow copy and assignment of EventListeners. The only use case is async identity checking, which should use EventListener::ID instead. 3) This cleanup means EventListener::blocked_requests no longer has to be mutable. (A clear sign that something was wrong with the previous scheme). The most important result is that it is no longer possible to have identity confusion between EventListeners, as all comparisons use EventListener::ID. This was the root cause of the leak from Issue 643025 . BUG= 643025 , 641958 Review-Url: https://codereview.chromium.org/2303113002 Cr-Commit-Position: refs/heads/master@{#417398} [modify] https://crrev.com/7cdfec136b896222345c88f56e89a2d40d53e49c/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc [modify] https://crrev.com/7cdfec136b896222345c88f56e89a2d40d53e49c/extensions/browser/api/web_request/web_request_api.cc [modify] https://crrev.com/7cdfec136b896222345c88f56e89a2d40d53e49c/extensions/browser/api/web_request/web_request_api.h
,
Sep 12 2016
,
Sep 15 2016
,
Sep 15 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f5f0196947ef79ab06a121329945ed3141c9b095 commit f5f0196947ef79ab06a121329945ed3141c9b095 Author: erikchen <erikchen@chromium.org> Date: Thu Sep 15 18:27:36 2016 [Merge to 2840] Fix semantics of ExtensionWebRequestEventRouter::EventListeners. > There are two types of EventListeners: those with web_view_instance_id and those > without. EventListeners with a web_view_instance_id need to always consider > embedder_process_id for equality comparisons. EventListeners without a > web_view_instance_id should sometimes ignore embedder_process_id (when being > removed) for equality comparisons. > > The previous code had two major issues: > 1) EventListeners were stored in a std::set. std:set requires a stable sort > operator. There is no way to create a stable sort operator that sometimes > ignores embedder_process_id. > > Aside: std::set is almost never the right container to use from a performance > perspective. In this case, the code frequently needs to iterate through the > entire container away, so I've changed the container to be a std::vector (we > expect the total number of elements to be small). If we find performance > issues, we should switch to std::unordered_set. > > 2) EventListeners need to be messaged asynchronously, but it's possible that > the EventListener is no longer registered by the time the callback comes > around. The previous code would make a copy of the EventListener, and then try > to "find" the original copy in the callback. Once again, this requires clear > semantics for equality. > > The new code makes several changes: > 1) Create the class EventListener::ID. As its name implies, this uniquely > identifies an EventListener. > > 2) Disallow copy and assignment of EventListeners. The only use case is async > identity checking, which should use EventListener::ID instead. > > 3) This cleanup means EventListener::blocked_requests no longer has to be > mutable. (A clear sign that something was wrong with the previous scheme). > > The most important result is that it is no longer possible to have identity > confusion between EventListeners, as all comparisons use EventListener::ID. This > was the root cause of the leak from Issue 643025 . > > BUG= 643025 , 641958 > > Review-Url: https://codereview.chromium.org/2303113002 > Cr-Commit-Position: refs/heads/master@{#417398} > (cherry picked from commit 7cdfec136b896222345c88f56e89a2d40d53e49c) Review URL: https://codereview.chromium.org/2344993002 . Cr-Commit-Position: refs/branch-heads/2840@{#379} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/f5f0196947ef79ab06a121329945ed3141c9b095/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc [modify] https://crrev.com/f5f0196947ef79ab06a121329945ed3141c9b095/extensions/browser/api/web_request/web_request_api.cc [modify] https://crrev.com/f5f0196947ef79ab06a121329945ed3141c9b095/extensions/browser/api/web_request/web_request_api.h
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a67f30a6b303582d930417197f4a351901c6898a commit a67f30a6b303582d930417197f4a351901c6898a Author: Erik Chen <erikchen@chromium.org> Date: Tue Sep 06 23:56:27 2016 [Merge to 2840] Revert of Fix WebRequest's EventListener::operator< (patchset #3 id:40001 of https://codereview.chromium.org/2121873002/ ) > Reason for revert: > This may have caused a perf regression. Reverting to verify. If it did not, I'll reland next week. > > BUG= 641958 , 643025 > > Original issue's description: > > Fix WebRequest's EventListener::operator< > > > > std::set's count() method is expected to return 0 or 1. But due > > to the fact that EventListener::operator< conditionally ignored > > |embedder_process_id|, the count method could return more than 1 > > (e.g. 2) in release builds on Windows (VC++), and fail an assertion. > > > > This assertion failure shows that the assumption about the uniqueness > > of EventListeners (while ignoring the |embedder_process_id| value) does > > not hold, i.e. there may be more than one non-webview EventListeners > > with the same |extension_id| and |sub_event_name|, but different > > |embedder_process_id|. > > > > To fix this, this patch removes the conditional exclusion of > > |embedder_process_id| from operator< and looks up |embedder_process_id| > > before searching through the set. > > > > BUG=589735 > > TEST=./unit_tests --gtest_filter=ExtensionWebRequestTest.AddAndRemoveListeners > > > > Committed: https://crrev.com/c89943b168ef2ccc51f3ec0720e552f36ec31351 > > Cr-Commit-Position: refs/heads/master@{#404456} > > TBR=rockot@chromium.org,rob@robwu.nl > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=589735 > > Review-Url: https://codereview.chromium.org/2299373002 > Cr-Commit-Position: refs/heads/master@{#416094} > (cherry picked from commit e579d519620e6583ddc304f22c4c3c71dabbbe20) Review URL: https://codereview.chromium.org/2317923002 . Cr-Commit-Position: refs/branch-heads/2840@{#192} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/a67f30a6b303582d930417197f4a351901c6898a/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc [modify] https://crrev.com/a67f30a6b303582d930417197f4a351901c6898a/extensions/browser/api/web_request/web_request_api.cc [modify] https://crrev.com/a67f30a6b303582d930417197f4a351901c6898a/extensions/browser/api/web_request/web_request_api.h
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f5f0196947ef79ab06a121329945ed3141c9b095 commit f5f0196947ef79ab06a121329945ed3141c9b095 Author: erikchen <erikchen@chromium.org> Date: Thu Sep 15 18:27:36 2016 [Merge to 2840] Fix semantics of ExtensionWebRequestEventRouter::EventListeners. > There are two types of EventListeners: those with web_view_instance_id and those > without. EventListeners with a web_view_instance_id need to always consider > embedder_process_id for equality comparisons. EventListeners without a > web_view_instance_id should sometimes ignore embedder_process_id (when being > removed) for equality comparisons. > > The previous code had two major issues: > 1) EventListeners were stored in a std::set. std:set requires a stable sort > operator. There is no way to create a stable sort operator that sometimes > ignores embedder_process_id. > > Aside: std::set is almost never the right container to use from a performance > perspective. In this case, the code frequently needs to iterate through the > entire container away, so I've changed the container to be a std::vector (we > expect the total number of elements to be small). If we find performance > issues, we should switch to std::unordered_set. > > 2) EventListeners need to be messaged asynchronously, but it's possible that > the EventListener is no longer registered by the time the callback comes > around. The previous code would make a copy of the EventListener, and then try > to "find" the original copy in the callback. Once again, this requires clear > semantics for equality. > > The new code makes several changes: > 1) Create the class EventListener::ID. As its name implies, this uniquely > identifies an EventListener. > > 2) Disallow copy and assignment of EventListeners. The only use case is async > identity checking, which should use EventListener::ID instead. > > 3) This cleanup means EventListener::blocked_requests no longer has to be > mutable. (A clear sign that something was wrong with the previous scheme). > > The most important result is that it is no longer possible to have identity > confusion between EventListeners, as all comparisons use EventListener::ID. This > was the root cause of the leak from Issue 643025 . > > BUG= 643025 , 641958 > > Review-Url: https://codereview.chromium.org/2303113002 > Cr-Commit-Position: refs/heads/master@{#417398} > (cherry picked from commit 7cdfec136b896222345c88f56e89a2d40d53e49c) Review URL: https://codereview.chromium.org/2344993002 . Cr-Commit-Position: refs/branch-heads/2840@{#379} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/f5f0196947ef79ab06a121329945ed3141c9b095/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc [modify] https://crrev.com/f5f0196947ef79ab06a121329945ed3141c9b095/extensions/browser/api/web_request/web_request_api.cc [modify] https://crrev.com/f5f0196947ef79ab06a121329945ed3141c9b095/extensions/browser/api/web_request/web_request_api.h |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by erikc...@chromium.org
, Sep 1 2016From the sample, the following line near the beginning of the ridiculous STL stack: + ! : | + ! : | ! : 366 std::__1::__vector_base<extensions::ExtensionWebRequestEventRouter::EventListener, std::__1::allocator<extensions::ExtensionWebRequestEventRouter::EventListener> >::~__vector_base() (in Google Chrome Framework) load address 0x103667000 + 0x12346c8 [weak_ptr.h:246] suggests that the problem is that we are destroying a vector of ExtensionWebRequestEventRouter::EventListener, which then destroys a *huge* tree of unsigned long longs. Looking at the struct, this may be "mutable std::set<uint64_t> blocked_requests;"? Is it possible that this set grows without bound and eventually becomes crazy huge?