Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

th/dbus_cache_properties #82

Merged
merged 10 commits into from Mar 27, 2018
Merged

th/dbus_cache_properties #82

merged 10 commits into from Mar 27, 2018

Conversation

thom311
Copy link
Member

@thom311 thom311 commented Mar 24, 2018

Cache property GVariants for D-Bus and make more use of CList to track devices in manager and NMWifiAP

@thom311
Copy link
Member Author

thom311 commented Mar 25, 2018

@balrog-kun, some changes to IWD code here. Does it look OK?

bengal
bengal reviewed Mar 26, 2018
View reviewed changes
src/nm-manager.c
if ( !ac
|| !NM_IS_ACTIVE_CONNECTION (ac)
|| c_list_is_empty (&ac->active_connections_lst))
return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these be assertions? If nm_dbus_manager_lookup_object() returns a non-NULL pointer and it's not an active connection I think there is a programming error somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I am in favor of assertions, but in this case, I think it's too much. Note already before, the code allowed for the possibility that the path may not exist. The possibility of a non-existing path, means the caller apparently has no control about the path he looks up, so asserting against it is wrong. Maybe in this case, for active_connection_get_by_path() you are right. But for example nm_wifi_ap_lookup_for_device() does something similar, and that is called with a path provided by the user (untrusted).

src/nm-manager.c
if ( !device
|| !NM_IS_DEVICE (device)
|| c_list_is_empty (&device->devices_lst))
return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above, I think NM_IS_DEVICE (device) and c_list_is_empty (&device->devices_lst) should be assertions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See how nm_checkpoint_manager_create() looks paths up from untrusted source.

src/devices/nm-device.h
@@ -174,6 +174,7 @@ struct _NMDevicePrivate;
struct _NMDevice {
NMDBusObject parent;
struct _NMDevicePrivate *_priv;
CList devices_lst;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core: track devices in manager via embedded C-List

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extended commit message. Ok?

@thom311
core/dbus: cache variants for D-Bus exported properties
640736f 
GVariant is immutable and can nicely be shared and cached.
Cache the property variants. This makes getting the properties
faster, at the expense of using some extra memory.

Tested with https://tratt.net/laurie/src/multitime/

  $ multitime -n 200 -s 0 bash -c 'echo -n .; exec busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects &>/dev/null'
  #                     Mean                Std.Dev.    Min         Median      Max
  # real(before)        0.013+/-0.0000      0.001       0.012       0.013       0.019
  # real(after)         0.013+/-0.0000      0.002       0.011       0.012       0.034

  $ multitime -n 100 -s 0 bash -c 'for i in {1..5}; do busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects &>/dev/null & done; wait; echo -n .'
  #                     Mean                Std.Dev.    Min         Median      Max
  # real(before)        0.040+/-0.0000      0.002       0.037       0.040       0.049
  # real(after)         0.037+/-0.0000      0.002       0.034       0.036       0.045

  $ multitime -n 30 -s 0 bash -c 'for i in {1..100}; do busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects &>/dev/null & done; wait; echo -n .'
  #                     Mean                Std.Dev.    Min         Median      Max
  # real(before)        0.704+/-0.0000      0.016       0.687       0.701       0.766
  # real(after)         0.639+/-0.0000      0.013       0.622       0.637       0.687

  $ multitime -n 200 -s 0 bash -c 'echo -n .; exec nmcli &>/dev/null'
  #                     Mean                Std.Dev.    Min         Median      Max
  # real(before)        0.092+/-0.0000      0.005       0.081       0.092       0.119
  # real(after)         0.092+/-0.0000      0.005       0.080       0.091       0.123

  $ multitime -n 100 -s 0 bash -c 'for i in {1..5}; do nmcli &>/dev/null & done; wait; echo -n .'
  #                     Mean                Std.Dev.    Min         Median      Max
  # real(before)        0.436+/-0.0000      0.043       0.375       0.424       0.600
  # real(after)         0.413+/-0.0000      0.022       0.380       0.410       0.558

  $ multitime -n 20 -s 0 bash -c 'for i in {1..100}; do nmcli &>/dev/null & done; wait; echo -n .'
  #                     Mean                Std.Dev.    Min         Median      Max
  # real(before)        8.796+/-0.1070      0.291       8.073       8.818       9.247
  # real(after)         8.736+/-0.0893      0.243       8.017       8.780       9.101

The time savings are small, but that is because caching mostly speeds up
the GetManagedObjects calls, and that is only a small part of the entire
nmcli call from client side.
@thom311
core/dbus: manually inline helper function to emit InterfacesAdded si…
987c584 
…gnal
@thom311
core: track devices in manager via embedded CList
4a705e1 
Instead of using a GSList for tracking the devices, use a CList.
I think a CList is in most cases the more suitable data structure
then GSList:

 - you can find out in O(1) whether the object is linked. That
   is nice, for example to assert in NMDevice's destructor that
   the object was unlinked, and we will use that later in
   nm_manager_get_device_by_path().
 - you can unlink the element in O(1) and you can unlink the
   element without having access to the link's head
 - Contrary to GSList, this does not require an extra slice
   allocation for the link node. It quite possibliy consumes
   slightly less memory because the CList structure is embedded
   in a struct that we already allocate. Even if slice allocation
   would be perfect to only consume 2*sizeof(gpointer) for the link
   note, it would at most be as-good as CList. Quite possibly,
   there is an overhead though.
 - CList possibly has better memory locality, because the link
   structure and the data are close to each other.

Something which could be seen as disavantage, is that with CList
one device can only be tracked in one NMManager instance at a time.
But that is fine. There exists only one NMManager instance for now,
and even if we would ever introduce multiple managers, we probably
would not associate one NMDevice instance with multiple managers.

The advantages are arguably not huge, but CList is IMHO clearly the
more suited data structure. No need to stick to a suboptimal data
structure for the job. Refactor it.
@thom311
manager: convert hwaddr to binary once in find_device_by_permanent_hw…
199f2df 
…_addr()

For comparing MAC addresses, they anyway have to be normalized
to binary. Convert it once outside the loop and pass the binary
form to nm_utils_hwaddr_matches(). Otherwise, we need to re-convert
it every time.
@thom311
core: rework lookup for exported objects by path to use index
9fafd26 
We already track an index of exported objects in NMDBusManager.
Actually, that index was unused previously. We either could drop
it, or use it. Let's use it.
@thom311
wifi: rework tracking of wifi-aps to use CList
d7b1a91 
- no longer track APs in a hash table with their exported path
  as key. The exported path is already tracked by NMDBusManager's
  lookup index, so we can reuse that for fast lookup by path. Otherwise,
  track the APs in a CList per device.

- as we now track APs in a CList, their order is well defined.
  We no longer need to sort APs and obsoletes nm_wifi_aps_get_sorted()
  and simplifies nm_wifi_aps_find_first_compatible().
@thom311
shared: add nm_utils_hash_keys_to_array() helper
938d9a8
@thom311
config/trivial: rename dns_config local variable
0f1dc3b 
The variable serves a similar purpose, but is called "dns",
"conf", and "dns_config". Choose one name: "dns_config".
@thom311
config: cleanup fields in NMGlobalDnsConfig
cd48bc7 
- consistently set options, searches, domains fields to %NULL,
  if there are no values.

- in nm_global_dns_config_update_checksum(), ensure that we uniquely
  hash values. E.g. a config with "searches[a], options=[b]" should
  hash differently from "searches=[ab], options=[]".

- in nm_global_dns_config_to_dbus(), reuse the sorted domain list.
  We already have it, and it guarantees a consistent ordering of
  fields.

- in global_dns_domain_from_dbus(), fix memleaks if D-Bus strdict
  contains duplicate entries.
@thom311
all: use nm_utils_hash_keys_to_array()
e49a329
@bengal
Copy link
Contributor

bengal commented Mar 27, 2018

LGTM now

@thom311 thom311 merged commit e49a329 into master Mar 27, 2018
@thom311
Copy link
Member Author

thom311 commented Mar 27, 2018

Thanks. Merged

@thom311 thom311 deleted the th/dbus-cache-properties branch March 27, 2018 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bengal bengal bengal left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
2 participants
@thom311 @bengal