Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14813021:
Observe property updates for all Network Services (Closed)
|
Created:
7 years, 7 months ago by stevenjb Modified:
7 years, 7 months ago CC:
chromium-reviews, gauravsh+watch_chromium.org, gspencer+watch_chromium.org, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionObserve property updates for all Network Services
BUG=239200
R=pneubeck@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200024
Patch Set 1 #
Total comments: 5
Messages
Total messages: 12 (0 generated)
Should the event log buffer size also be increased? Have you checked how much extra event log this ends up adding in dense WiFi environments, and whether 1000 lines is still sufficient to get events from a reasonable time window?
Where do we still need to distinguish watched and not-watched services? We differentiate visible and configured networks (which overlap in visible+configured networks). Will we use the watch flag for that? Then we should rename it to "visible" in the stubs and handle it accordingly. https://codereview.chromium.org/14813021/diff/1/chromeos/network/shill_proper... File chromeos/network/shill_property_handler.cc (right): https://codereview.chromium.org/14813021/diff/1/chromeos/network/shill_proper... chromeos/network/shill_property_handler.cc:263: // UpdateObserved used to use kServiceWatchListProperty for TYPE_NETWORK, shouldn't this be a method comment? https://codereview.chromium.org/14813021/diff/1/chromeos/network/shill_proper... chromeos/network/shill_property_handler.cc:325: if (new_observed.size() >= kMaxObserved) This is a serious restriction which should be documented at the NetworkStateHandler class description. I'm still not sure that this is the right thing to do. Memory overhead cannot be the argument as Shill also stores these networks (thus memory usage at most doubles). The number of updates might already be diminishingly small (only triggered by scans). If it's still too much work, how does Shill handle it? Is your concern related to the number of DBus registrations? DBus provides a solution to that. On the other hand, we might wonder at some time why we don't see certain networks in the UI.
On 2013/05/12 19:05:18, gauravsh wrote: > Should the event log buffer size also be increased? > > Have you checked how much extra event log this ends up adding in dense WiFi > environments, and whether 1000 lines is still sufficient to get events from a > reasonable time window? This shouldn't impact log noise that significantly, Strength updates (which should be the only high frequency updates) only occur after a scan request. If it becomes an issue we can bump that number up.
On 2013/05/13 10:21:03, pneubeck wrote: > Where do we still need to distinguish watched and not-watched services? > > We differentiate visible and configured networks (which overlap in > visible+configured networks). Will we use the watch flag for that? Then we > should rename it to "visible" in the stubs and handle it accordingly. > With this changed we effectively ignore "Watched" networks. I left the stub code as-is in case we change back later. I'm not sure I understand your second comment/question. "Visible" networks = networks in the Manager.Services list = Visible networks + connected networks. Configured but not visible networks do not show up in this list. "Hidden" networks will only show up if they are connected to. "Remembered" networks = networks that have been configured. They may or may not be visible. Currently NetworkState does not know or care whether or not a Visible network is Remembered. We do know if it is "Connectable" which means it either has been configured (and therefore is Remembered) or does not require configuration.
https://codereview.chromium.org/14813021/diff/1/chromeos/network/shill_proper... File chromeos/network/shill_property_handler.cc (right): https://codereview.chromium.org/14813021/diff/1/chromeos/network/shill_proper... chromeos/network/shill_property_handler.cc:263: // UpdateObserved used to use kServiceWatchListProperty for TYPE_NETWORK, On 2013/05/13 10:21:03, pneubeck wrote: > shouldn't this be a method comment? No, it's really an implementation detail. I'd rather have it here. https://codereview.chromium.org/14813021/diff/1/chromeos/network/shill_proper... chromeos/network/shill_property_handler.cc:325: if (new_observed.size() >= kMaxObserved) On 2013/05/13 10:21:03, pneubeck wrote: > This is a serious restriction which should be documented at the > NetworkStateHandler class description. > > I'm still not sure that this is the right thing to do. > Memory overhead cannot be the argument as Shill also stores these networks (thus > memory usage at most doubles). > The number of updates might already be diminishingly small (only triggered by > scans). If it's still too much work, how does Shill handle it? > Is your concern related to the number of DBus registrations? DBus provides a > solution to that. > > On the other hand, we might wonder at some time why we don't see certain > networks in the UI. I don't think this is a big of a restriction as you think it is. We will still list all services and fetch their initial properties. "Interesting" services such as connecting services will always be listed at the top, see the comment for kMaxObserverd. This is just a limit on: a) The number of dbus registrations (which, yes we could improve, but that gets complicated quickly) b) The potential number of observer updates, which have non-trivial UI overhead. The only practical effect is that we will not see Strength updates for networks we don't really care much about. It's not an important limit, we could almost certainly remove it, I would just prefer to avoid a situation where an edge case with 1000+ visible networks might cause unrecoverable performance bottlenecks. This seems like a simpler solution than optimizing for an edge case.
On 2013/05/13 21:30:25, stevenjb (chromium) wrote: > On 2013/05/13 10:21:03, pneubeck wrote: > > Where do we still need to distinguish watched and not-watched services? > > > > We differentiate visible and configured networks (which overlap in > > visible+configured networks). Will we use the watch flag for that? Then we > > should rename it to "visible" in the stubs and handle it accordingly. > > > > With this changed we effectively ignore "Watched" networks. I left the stub code > as-is in case we change back later. > > I'm not sure I understand your second comment/question. > > "Visible" networks = networks in the Manager.Services list = Visible networks + > connected networks. Configured but not visible networks do not show up in this > list. "Hidden" networks will only show up if they are connected to. > > "Remembered" networks = networks that have been configured. They may or may not > be visible. > > Currently NetworkState does not know or care whether or not a Visible network is > Remembered. We do know if it is "Connectable" which means it either has been > configured (and therefore is Remembered) or does not require configuration. My comment was not specific to NetworkStatHandler but also related to the DBus clients (e.g. ShillServiceClient::TestInterface::AddService). In Shill a service can exist for networks that are not-visible, not-connected but configured. We have to reflect that in our clients.
https://codereview.chromium.org/14813021/diff/1/chromeos/network/shill_proper... File chromeos/network/shill_property_handler.cc (right): https://codereview.chromium.org/14813021/diff/1/chromeos/network/shill_proper... chromeos/network/shill_property_handler.cc:325: if (new_observed.size() >= kMaxObserved) On 2013/05/13 21:31:40, stevenjb (chromium) wrote: > On 2013/05/13 10:21:03, pneubeck wrote: > > This is a serious restriction which should be documented at the > > NetworkStateHandler class description. > > > > I'm still not sure that this is the right thing to do. > > Memory overhead cannot be the argument as Shill also stores these networks > (thus > > memory usage at most doubles). > > The number of updates might already be diminishingly small (only triggered by > > scans). If it's still too much work, how does Shill handle it? > > Is your concern related to the number of DBus registrations? DBus provides a > > solution to that. > > > > On the other hand, we might wonder at some time why we don't see certain > > networks in the UI. > > I don't think this is a big of a restriction as you think it is. > > We will still list all services and fetch their initial properties. > > "Interesting" services such as connecting services will always be listed at the > top, see the comment for kMaxObserverd. > > This is just a limit on: > a) The number of dbus registrations (which, yes we could improve, but that gets > complicated quickly) > b) The potential number of observer updates, which have non-trivial UI > overhead. > > The only practical effect is that we will not see Strength updates for networks > we don't really care much about. > > It's not an important limit, we could almost certainly remove it, I would just > prefer to avoid a situation where an edge case with 1000+ visible networks might > cause unrecoverable performance bottlenecks. > > This seems like a simpler solution than optimizing for an edge case. > Ok. Here is the conflict we run into with this: - Configure a network XY. - On Startup be within close range of XY's AP. - NetworkStateHandler's initial update will fetch the high signalstrength for XY. - Move away from XY's AP => NetworkStateHandler stops observing XY. - Go to the settings page and modify some property of XY that ends up in the network state, like IP config. - NetworkStateHandler will not be notified => XY's NetworkState is stale - Because it's configured and visible, some UI may expose XY's stale NetworkState In general, any configurable property (e.g. security, guid) that is present in NetworkState can potentially be inconsistent with the last configuration. Depending on where we use which properties of NetworkState, this may or may not end up in an actual bug; but at least it's a subtle unexpected behavior. If we can avoid it now, we should do so. I see three alternatives: - Remove the limit - Always observe configured networks independent of the limit - Remove all configurable properties from NetworkState (and kNameProperty might be one of them in the future).
On 2013/05/14 14:12:32, pneubeck wrote: > > Ok. Here is the conflict we run into with this: > > - Configure a network XY. > - On Startup be within close range of XY's AP. > - NetworkStateHandler's initial update will fetch the high signalstrength for > XY. > - Move away from XY's AP => NetworkStateHandler stops observing XY. With this change that would only happen if XY's index in the list > 100, but yes that can still happen. > - Go to the settings page and modify some property of XY that ends up in the > network state, like IP config. > - NetworkStateHandler will not be notified => XY's NetworkState is stale I would expect Shill to bump the order of a recently configured service in the list, but if we do not want to rely on Shill for that behavior, we can easily have NetworkConfigurationHandler inform NetworkStateHandler to observe XY if it is not observed. I can talk to Paul about what Shill does, and if necessary add that in a separate CL. > - Because it's configured and visible, some UI may expose XY's stale > NetworkState > > In general, any configurable property (e.g. security, guid) that is present in > NetworkState can potentially be inconsistent with the last configuration. > > Depending on where we use which properties of NetworkState, this may or may not > end up in an actual bug; but at least it's a subtle unexpected behavior. If we > can avoid it now, we should do so. > > I see three alternatives: > - Remove the limit I prefer not to, but would not find that especially onerous. > - Always observe configured networks independent of the limit I prefer this solution. If we change a configuration we should either flag the service so that if it is in Manager.Services we observe it, or even easier just request an immediate update of the service properties (which, now that I think about it may work best). > - Remove all configurable properties from NetworkState (and kNameProperty might > be one of them in the future). I think this would make a lot of the UI unresponsive and difficult to write, so this does not seem like a good option.
On 2013/05/14 15:22:47, stevenjb (chromium) wrote: > On 2013/05/14 14:12:32, pneubeck wrote: > > > > Ok. Here is the conflict we run into with this: > > > > - Configure a network XY. > > - On Startup be within close range of XY's AP. > > - NetworkStateHandler's initial update will fetch the high signalstrength for > > XY. > > - Move away from XY's AP => NetworkStateHandler stops observing XY. > > With this change that would only happen if XY's index in the list > 100, but yes > that can still happen. > > > - Go to the settings page and modify some property of XY that ends up in the > > network state, like IP config. > > - NetworkStateHandler will not be notified => XY's NetworkState is stale > > I would expect Shill to bump the order of a recently configured service in the > list, but if we do not want to rely on Shill for that behavior, we can easily > have NetworkConfigurationHandler inform NetworkStateHandler to observe XY if it > is not observed. I can talk to Paul about what Shill does, and if necessary add > that in a separate CL. > > > - Because it's configured and visible, some UI may expose XY's stale > > NetworkState > > > > In general, any configurable property (e.g. security, guid) that is present in > > NetworkState can potentially be inconsistent with the last configuration. > > > > Depending on where we use which properties of NetworkState, this may or may > not > > end up in an actual bug; but at least it's a subtle unexpected behavior. If we > > can avoid it now, we should do so. > > > > I see three alternatives: > > - Remove the limit > I prefer not to, but would not find that especially onerous. > > > - Always observe configured networks independent of the limit > I prefer this solution. If we change a configuration we should either flag the > service so that if it is in Manager.Services we observe it, or even easier just > request an immediate update of the service properties (which, now that I think > about it may work best). An update request immediately after each configuration, appears fine to me. We shouldn't forget that... The low chance of running into this issue while NetworkLibrary is still used, should be acceptable. > > > - Remove all configurable properties from NetworkState (and kNameProperty > might > > be one of them in the future). > I think this would make a lot of the UI unresponsive and difficult to write, so > this does not seem like a good option.
lgtm
Message was sent while issue was closed.
Committed patchset #1 manually as r200024 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
