Skip to content
  • Thomas Haller's avatar
    core/dbus: rework D-Bus implementation to use lower layer GDBusConnection API · 297d4985
    Thomas Haller authored
    Previously, we used the generated GDBusInterfaceSkeleton types and glued
    them via the NMExportedObject base class to our NM types. We also used
    GDBusObjectManagerServer.
    
    Don't do that anymore. The resulting code was more complicated despite (or
    because?) using generated classes. It was hard to understand, complex, had
    ordering-issues, and had a runtime and memory overhead.
    
    This patch refactors this entirely and uses the lower layer API GDBusConnection
    directly. It replaces the generated code, GDBusInterfaceSkeleton, and
    GDBusObjectManagerServer. All this is now done by NMDbusObject and NMDBusManager
    and static descriptor instances of type GDBusInterfaceInfo.
    
    This adds a net plus of more then 1300 lines of hand written code. I claim
    that this implementation is easier to understand. Note that previously we
    also required extensive and complex glue code to bind our objects to the
    generated skeleton objects. Instead, now glue our objects directly to
    GDBusConnection. The result is more immediate and gets rid of layers of
    code in between.
    Now that the D-Bus glue us more under our control, we can address issus and
    bottlenecks better, instead of adding code to bend the generated skeletons
    to our needs.
    
    Note that the current implementation now only supports one D-Bus connection.
    That was effectively the case already, although there were places (and still are)
    where the code pretends it could also support connections from a private socket.
    We dropped private socket support mainly because it was unused, untested and
    buggy, but also because GDBusObjectManagerServer could not export the same
    objects on multiple connections. Now, it would be rather straight forward to
    fix that and re-introduce ObjectManager on each private connection. But this
    commit doesn't do that yet, and the new code intentionally supports only one
    D-Bus connection.
    Also, the D-Bus startup was simplified. There is no retry, either nm_dbus_manager_start()
    succeeds, or it detects the initrd case. In the initrd case, bus manager never tries to
    connect to D-Bus. Since the initrd scenario is not yet used/tested, this is good enough
    for the moment. It could be easily extended later, for example with polling whether the
    system bus appears (like was done previously). Also, restart of D-Bus daemon isn't
    supported either -- just like before.
    
    Note how NMDBusManager now implements the ObjectManager D-Bus interface
    directly.
    
    Also, this fixes race issues in the server, by no longer delaying
    PropertiesChanged signals. NMExportedObject would collect changed
    properties and send the signal out in idle_emit_properties_changed()
    on idle. This messes up the ordering of change events w.r.t. other
    signals and events on the bus. Note that not only NMExportedObject
    messed up the ordering. Also the generated code would hook into
    notify() and process change events in and idle handle, exhibiting the
    same ordering issue too.
    No longer do that. PropertiesChanged signals will be sent right away
    by hooking into dispatch_properties_changed(). This means, changing
    a property in quick succession will no longer be combined and is
    guaranteed to emit signals for each individual state. Quite possibly
    we emit now more PropertiesChanged signals then before.
    However, we are now able to group a set of changes by using standard
    g_object_freeze_notify()/g_object_thaw_notify(). We probably should
    make more use of that.
    
    Also, now that our signals are all handled in the right order, we
    might find places where we still emit them in the wrong order. But that
    is then due to the order in which our GObjects emit signals, not due
    to an ill behavior of the D-Bus glue. Possibly we need to identify
    such ordering issues and fix them.
    
    Numbers (for contrib/rpm --without debug on x86_64):
    
    - the patch changes the code size of NetworkManager by
      - 2809360 bytes
      + 2537528 bytes (-9.7%)
    
    - Runtime measurements are harder because there is a large variance
      during testing. In other words, the numbers are not reproducible.
      Currently, the implementation performs no caching of GVariants at all,
      but it would be rather simple to add it, if that turns out to be
      useful.
      Anyway, without strong claim, it seems that the new form tends to
      perform slightly better. That would be no surprise.
    
      $ time (for i in {1..1000}; do nmcli >/dev/null || break; echo -n .;  done)
      - real    1m39.355s
      + real    1m37.432s
    
      $ time (for i in {1..2000}; do busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects > /dev/null || break; echo -n .; done)
      - real    0m26.843s
      + real    0m25.281s
    
    - Regarding RSS size, just looking at the processes in similar
      conditions, doesn't give a large difference. On my system they
      consume about 19MB RSS. It seems that the new version has a
      slightly smaller RSS size.
      - 19356 RSS
      + 18660 RSS
    297d4985