Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12220128:
Shill: Delay the ConnectToSignal call until an observer is added. (Closed)
|
Created:
7 years, 10 months ago by deymo Modified:
7 years, 10 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionShill: Delay the ConnectToSignal call until an observer is added.
This patch delayes the call to ObjectProxy::ConnectToSignal made from the
ShillClientHelper in order to call it only when there's an observer for the
signal. If no observer is currently added, as in the case of a ShillClientHelper
used only for method calls, this fix will efectively cause the helper to not
add an unnecessary match rule in the dbus connection. This notably reduces the
number of match rules used by Shill to only the number of connected networks
instead of the number of available networks.
BUG=chromium:170182
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182525
Patch Set 1 #
Total comments: 4
Patch Set 2 : nits addressed #
Total comments: 4
Patch Set 3 : 2nd round of nits. #Patch Set 4 : apply issues on try bots, patch rebased. #
Messages
Total messages: 14 (0 generated)
Hi. This is my 4th CL attempting to solve/address/circumvent the same problem. This is CL is a subset of the last patch in this other CL https://codereview.chromium.org/12220025/ . The goal here is to simply avoid using the AddMatch/RemoveMatch when those are not needed (i.e., when only methods are called). The price we pay for this is that AddPropertyChangedObserver may block now, but this is something we need to pay anyway if we want to call AddMatch only when we add an observer. No matter the interface or intricate callback mess we can make here, if adding an observer calls AddMatch at the end, it will block. I think this patch fixes the problem with the smallest change, and we can request it to be merged in 26. Given that we didn't have consensus for the previous patch, I would like to commit a short-term solution that fixes the bug instead of iterating what *should* we do without getting to the point. deymo.
lgtm if it is practically confirmed that this change is worth merging. How long does it take to reach the match rule limit after this change? In crbug.com/170182, it is said that it takes 25h with the current implementation. If we can make the rate of increase so slow that takes 200h or longer to reach the limit, I think it's worth merging. https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... File chromeos/dbus/shill_client_helper.cc (right): https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... chromeos/dbus/shill_client_helper.cc:55: if (observer_list_.size() > 0) nit: if-statement without curly braces are not allowed for multiple line cases. https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... File chromeos/dbus/shill_client_helper.h (right): https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... chromeos/dbus/shill_client_helper.h:188: std::vector<std::string> monitored_interfaces_; nit: Since all users are using this class for a single class, just having a string, instead of vector<string>, may suffice. (maybe interface name should be passed to the ctor, though it can be done in a separate patch if we are going to merge this to M26) another-nit: How about renaming this variable to "interfaces_to_be_monitored_" or something? At least, interfaces are not observed as long as they are stored in this vector.
The main number of match rules added are because we call GetProperties for every network on every scan, and we have some wi-fi scan on the background even if we are connected to a given network. This implies that for every network that appears after a new scan (this also includes networks that go away and come back due signal issues) we call GetProperties which results in an useless AddMatch. In an office environment the limit is reach in about 25hs. This fix permits to call GetProperties without adding the match rule unless we click on "connect" for that network. So now it's irrelevant the number of available networks resulting from the scanning (they will create an ObjectProxy and a ClientHelper but not the match rule). I tested this in a real machine with a log message for every AddMatch call, and I only see those calls when I switch networks trough the UI, and not after forcing a scan while spoofing APs. PS: patch 2 with the nits included. https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... File chromeos/dbus/shill_client_helper.cc (right): https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... chromeos/dbus/shill_client_helper.cc:55: if (observer_list_.size() > 0) On 2013/02/13 04:01:31, hashimoto wrote: > nit: if-statement without curly braces are not allowed for multiple line cases. Done. https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... File chromeos/dbus/shill_client_helper.h (right): https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... chromeos/dbus/shill_client_helper.h:188: std::vector<std::string> monitored_interfaces_; 1st nit: The current interface doesn't prevent someone to call several times MonitorPropertyChanged with different interfaces. With this interface, even handling "one string or nothing" looks simpler with a vector. +1 to pass the interface name in the constructor (but in another CL). In that other case a boolean will be better. 2nd nit: Done. On 2013/02/13 04:01:31, hashimoto wrote: > nit: Since all users are using this class for a single class, just having a > string, instead of vector<string>, may suffice. (maybe interface name should be > passed to the ctor, though it can be done in a separate patch if we are going to > merge this to M26) > another-nit: How about renaming this variable to "interfaces_to_be_monitored_" > or something? At least, interfaces are not observed as long as they are stored > in this vector.
On 2013/02/13 05:41:07, deymo wrote: > The main number of match rules added are because we call GetProperties for every > network on every scan, and we have some wi-fi scan on the background even if we > are connected to a given network. This implies that for every network that > appears after a new scan (this also includes networks that go away and come back > due signal issues) we call GetProperties which results in an useless AddMatch. > In an office environment the limit is reach in about 25hs. > > This fix permits to call GetProperties without adding the match rule unless we > click on "connect" for that network. So now it's irrelevant the number of > available networks resulting from the scanning (they will create an ObjectProxy > and a ClientHelper but not the match rule). > > I tested this in a real machine with a log message for every AddMatch call, and > I only see those calls when I switch networks trough the UI, and not after > forcing a scan while spoofing APs. Sounds great. > > PS: patch 2 with the nits included. > > https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... > File chromeos/dbus/shill_client_helper.cc (right): > > https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... > chromeos/dbus/shill_client_helper.cc:55: if (observer_list_.size() > 0) > On 2013/02/13 04:01:31, hashimoto wrote: > > nit: if-statement without curly braces are not allowed for multiple line > cases. > > Done. > > https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... > File chromeos/dbus/shill_client_helper.h (right): > > https://codereview.chromium.org/12220128/diff/1/chromeos/dbus/shill_client_he... > chromeos/dbus/shill_client_helper.h:188: std::vector<std::string> > monitored_interfaces_; > 1st nit: The current interface doesn't prevent someone to call several times > MonitorPropertyChanged with different interfaces. With this interface, even > handling "one string or nothing" looks simpler with a vector. +1 to pass the > interface name in the constructor (but in another CL). In that other case a > boolean will be better. > > 2nd nit: Done. > > On 2013/02/13 04:01:31, hashimoto wrote: > > nit: Since all users are using this class for a single class, just having a > > string, instead of vector<string>, may suffice. (maybe interface name should > be > > passed to the ctor, though it can be done in a separate patch if we are going > to > > merge this to M26) > > another-nit: How about renaming this variable to "interfaces_to_be_monitored_" > > or something? At least, interfaces are not observed as long as they are > stored > > in this vector.
lgtm, only tiny nits. Please wait for owner approval from satorux or stevenjb. https://codereview.chromium.org/12220128/diff/4/chromeos/dbus/shill_client_he... File chromeos/dbus/shill_client_helper.cc (right): https://codereview.chromium.org/12220128/diff/4/chromeos/dbus/shill_client_he... chromeos/dbus/shill_client_helper.cc:38: // Excecutes all the pending MonitorPropertyChanged calls. nitty-nit: s/Executes/Execute/ Our style guide does not require "-s" for implementation comments. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Implementation... https://codereview.chromium.org/12220128/diff/4/chromeos/dbus/shill_client_he... chromeos/dbus/shill_client_helper.cc:39: for (std::vector<std::string>::iterator it = nit: "for (size_t i = 0; i < interfaces_to_be_monitored_.end(); ++i)" is more common style in our code base to iterate over a vector.
I'd defer to stevenjb@ who's more familiar with the code.
lgtm
Patch3 with the nits from hashimoto. https://codereview.chromium.org/12220128/diff/4/chromeos/dbus/shill_client_he... File chromeos/dbus/shill_client_helper.cc (right): https://codereview.chromium.org/12220128/diff/4/chromeos/dbus/shill_client_he... chromeos/dbus/shill_client_helper.cc:38: // Excecutes all the pending MonitorPropertyChanged calls. On 2013/02/13 05:56:00, hashimoto wrote: > nitty-nit: s/Executes/Execute/ > Our style guide does not require "-s" for implementation comments. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Implementation... Done. https://codereview.chromium.org/12220128/diff/4/chromeos/dbus/shill_client_he... chromeos/dbus/shill_client_helper.cc:39: for (std::vector<std::string>::iterator it = On 2013/02/13 05:56:00, hashimoto wrote: > nit: "for (size_t i = 0; i < interfaces_to_be_monitored_.end(); ++i)" is more > common style in our code base to iterate over a vector. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deymo@chromium.org/12220128/11001
Retried try job too often on linux_chromeos for step(s) aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, check_deps, chromeos_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, dbus_unittests, device_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, net_unittests, ppapi_unittests, printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deymo@chromium.org/12220128/16003
Retried try job too often on linux_aura for step(s) aura_unittests, browser_tests, compositor_unittests, content_browsertests, content_unittests, interactive_ui_tests, unit_tests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deymo@chromium.org/12220128/16003
Message was sent while issue was closed.
Change committed as 182525 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
