Bug 703612 - Make DBUS calls asynchronous to prevent slowness because of DBus daemon being overloaded in Battery API UPower backend. r=mounir
authorAndrea Marchesini <amarchesini@mozilla.com>
Fri, 02 Nov 2012 16:11:50 -0400
changeset 112191 2b55937c8352cc19f259ca27e04b7edab5501463
parent 112190 fb8e7b959173f9c90db8f4f01aae3feded542eba
child 112192 5fb99507203104187976d486cb239a9ac04f6d39
push id23798
push userryanvm@gmail.com
push dateSat, 03 Nov 2012 00:06:35 +0000
treeherdermozilla-central@6134edeea902 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmounir
bugs703612
milestone19.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 703612 - Make DBUS calls asynchronous to prevent slowness because of DBus daemon being overloaded in Battery API UPower backend. r=mounir
hal/linux/UPowerClient.cpp
--- a/hal/linux/UPowerClient.cpp
+++ b/hal/linux/UPowerClient.cpp
@@ -12,23 +12,16 @@
 /*
  * Helper that manages the destruction of glib objects as soon as they leave
  * the current scope.
  *
  * We are specializing nsAutoRef class.
  */
 
 template <>
-class nsAutoRefTraits<DBusGProxy> : public nsPointerRefTraits<DBusGProxy>
-{
-public:
-  static void Release(DBusGProxy* ptr) { g_object_unref(ptr); }
-};
-
-template <>
 class nsAutoRefTraits<GHashTable> : public nsPointerRefTraits<GHashTable>
 {
 public:
   static void Release(GHashTable* ptr) { g_hash_table_unref(ptr); }
 };
 
 using namespace mozilla::dom::battery;
 
@@ -66,23 +59,27 @@ private:
     eState_PendingCharge,
     eState_PendingDischarge
   };
 
   /**
    * Update the currently tracked device.
    * @return whether everything went ok.
    */
-  void UpdateTrackedDevice();
+  void UpdateTrackedDeviceSync();
 
   /**
    * Returns a hash table with the properties of aDevice.
    * Note: the caller has to unref the hash table.
    */
-  GHashTable* GetDeviceProperties(const gchar* aDevice);
+  GHashTable* GetDevicePropertiesSync(DBusGProxy* aProxy);
+  void GetDevicePropertiesAsync(DBusGProxy* aProxy);
+  static void GetDevicePropertiesCallback(DBusGProxy* aProxy,
+                                          DBusGProxyCall* aCall,
+                                          void* aData);
 
   /**
    * Using the device properties (aHashTable), this method updates the member
    * variable storing the values we care about.
    */
   void UpdateSavedInfo(GHashTable* aHashTable);
 
   /**
@@ -102,16 +99,19 @@ private:
   DBusGConnection* mDBusConnection;
 
   // The DBus proxy object to upower.
   DBusGProxy* mUPowerProxy;
 
   // The path of the tracked device.
   gchar* mTrackedDevice;
 
+  // The DBusGProxy for the tracked device.
+  DBusGProxy* mTrackedDeviceProxy;
+
   double mLevel;
   bool mCharging;
   double mRemainingTime;
 
   static UPowerClient* sInstance;
 
   static const guint sDeviceTypeBattery = 2;
   static const guint64 kUPowerUnknownRemainingTime = 0;
@@ -160,25 +160,26 @@ UPowerClient::GetInstance()
 
   return sInstance;
 }
 
 UPowerClient::UPowerClient()
   : mDBusConnection(nullptr)
   , mUPowerProxy(nullptr)
   , mTrackedDevice(nullptr)
+  , mTrackedDeviceProxy(nullptr)
   , mLevel(kDefaultLevel)
   , mCharging(kDefaultCharging)
   , mRemainingTime(kDefaultRemainingTime)
 {
 }
 
 UPowerClient::~UPowerClient()
 {
-  NS_ASSERTION(!mDBusConnection && !mUPowerProxy && !mTrackedDevice,
+  NS_ASSERTION(!mDBusConnection && !mUPowerProxy && !mTrackedDevice && !mTrackedDeviceProxy,
                "The observers have not been correctly removed! "
                "(StopListening should have been called)");
 }
 
 void
 UPowerClient::BeginListening()
 {
   GError* error = nullptr;
@@ -201,17 +202,17 @@ UPowerClient::BeginListening()
   dbus_connection_add_filter(dbusConnection, ConnectionSignalFilter, this,
                              nullptr);
 
   mUPowerProxy = dbus_g_proxy_new_for_name(mDBusConnection,
                                            "org.freedesktop.UPower",
                                            "/org/freedesktop/UPower",
                                            "org.freedesktop.UPower");
 
-  UpdateTrackedDevice();
+  UpdateTrackedDeviceSync();
 
   /*
    * TODO: we should probably listen to DeviceAdded and DeviceRemoved signals.
    * If we do that, we would have to disconnect from those in StopListening.
    * It's not yet implemented because it requires testing hot plugging and
    * removal of a battery.
    */
   dbus_g_proxy_add_signal(mUPowerProxy, "DeviceChanged", G_TYPE_STRING,
@@ -233,125 +234,164 @@ UPowerClient::StopListening()
       ConnectionSignalFilter, this);
 
   dbus_g_proxy_disconnect_signal(mUPowerProxy, "DeviceChanged",
                                  G_CALLBACK (DeviceChanged), this);
 
   g_free(mTrackedDevice);
   mTrackedDevice = nullptr;
 
+  if (mTrackedDeviceProxy) {
+    g_object_unref(mTrackedDeviceProxy);
+    mTrackedDeviceProxy = nullptr;
+  }
+
   g_object_unref(mUPowerProxy);
   mUPowerProxy = nullptr;
 
   dbus_g_connection_unref(mDBusConnection);
   mDBusConnection = nullptr;
 
   // We should now show the default values, not the latest we got.
   mLevel = kDefaultLevel;
   mCharging = kDefaultCharging;
   mRemainingTime = kDefaultRemainingTime;
 }
 
 void
-UPowerClient::UpdateTrackedDevice()
+UPowerClient::UpdateTrackedDeviceSync()
 {
   GType typeGPtrArray = dbus_g_type_get_collection("GPtrArray",
                                                    DBUS_TYPE_G_OBJECT_PATH);
   GPtrArray* devices = nullptr;
   GError* error = nullptr;
 
+  // Reset the current tracked device:
+  g_free(mTrackedDevice);
+  mTrackedDevice = nullptr;
+
+  // Reset the current tracked device proxy:
+  if (mTrackedDeviceProxy) {
+    g_object_unref(mTrackedDeviceProxy);
+    mTrackedDeviceProxy = nullptr;
+  }
+
   // If that fails, that likely means upower isn't installed.
   if (!dbus_g_proxy_call(mUPowerProxy, "EnumerateDevices", &error, G_TYPE_INVALID,
                          typeGPtrArray, &devices, G_TYPE_INVALID)) {
     g_printerr ("Error: %s\n", error->message);
-
-    mTrackedDevice = nullptr;
     g_error_free(error);
     return;
   }
 
   /*
    * We are looking for the first device that is a battery.
    * TODO: we could try to combine more than one battery.
    */
   for (guint i=0; i<devices->len; ++i) {
     gchar* devicePath = static_cast<gchar*>(g_ptr_array_index(devices, i));
-    nsAutoRef<GHashTable> hashTable(GetDeviceProperties(devicePath));
+
+    DBusGProxy* proxy = dbus_g_proxy_new_from_proxy(mUPowerProxy,
+                                                    "org.freedesktop.DBus.Properties",
+                                                    devicePath);
+
+    nsAutoRef<GHashTable> hashTable(GetDevicePropertiesSync(proxy));
 
     if (g_value_get_uint(static_cast<const GValue*>(g_hash_table_lookup(hashTable, "Type"))) == sDeviceTypeBattery) {
       UpdateSavedInfo(hashTable);
       mTrackedDevice = devicePath;
+      mTrackedDeviceProxy = proxy;
       break;
     }
 
+    g_object_unref(proxy);
     g_free(devicePath);
   }
 
-#if GLIB_MAJOR_VERSION >= 2 && GLIB_MINOR_VERSION >= 22
-    g_ptr_array_unref(devices);
-#else
-    g_ptr_array_free(devices, true);
-#endif
+  g_ptr_array_free(devices, true);
 }
 
 /* static */ void
 UPowerClient::DeviceChanged(DBusGProxy* aProxy, const gchar* aObjectPath, UPowerClient* aListener)
 {
+  if (!aListener->mTrackedDevice) {
+    return;
+  }
+
 #if GLIB_MAJOR_VERSION >= 2 && GLIB_MINOR_VERSION >= 16
   if (g_strcmp0(aObjectPath, aListener->mTrackedDevice)) {
 #else
   if (g_ascii_strcasecmp(aObjectPath, aListener->mTrackedDevice)) {
 #endif
     return;
   }
 
-  nsAutoRef<GHashTable> hashTable(aListener->GetDeviceProperties(aObjectPath));
-  aListener->UpdateSavedInfo(hashTable);
-
-  hal::NotifyBatteryChange(hal::BatteryInformation(aListener->mLevel,
-                                                   aListener->mCharging,
-                                                   aListener->mRemainingTime));
+  aListener->GetDevicePropertiesAsync(aListener->mTrackedDeviceProxy);
 }
 
 /* static */ DBusHandlerResult
 UPowerClient::ConnectionSignalFilter(DBusConnection* aConnection,
                                      DBusMessage* aMessage, void* aData)
 {
   if (dbus_message_is_signal(aMessage, DBUS_INTERFACE_LOCAL, "Disconnected")) {
     static_cast<UPowerClient*>(aData)->StopListening();
     // We do not return DBUS_HANDLER_RESULT_HANDLED here because the connection
     // might be shared and some other filters might want to do something.
   }
 
   return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 }
 
 GHashTable*
-UPowerClient::GetDeviceProperties(const gchar* aDevice)
+UPowerClient::GetDevicePropertiesSync(DBusGProxy* aProxy)
 {
-  nsAutoRef<DBusGProxy> proxy(dbus_g_proxy_new_for_name(mDBusConnection,
-                                                        "org.freedesktop.UPower",
-                                                        aDevice,
-                                                        "org.freedesktop.DBus.Properties"));
-
   GError* error = nullptr;
   GHashTable* hashTable = nullptr;
   GType typeGHashTable = dbus_g_type_get_map("GHashTable", G_TYPE_STRING,
                                             G_TYPE_VALUE);
-  if (!dbus_g_proxy_call(proxy, "GetAll", &error, G_TYPE_STRING,
+  if (!dbus_g_proxy_call(aProxy, "GetAll", &error, G_TYPE_STRING,
                          "org.freedesktop.UPower.Device", G_TYPE_INVALID,
                          typeGHashTable, &hashTable, G_TYPE_INVALID)) {
     g_printerr("Error: %s\n", error->message);
     g_error_free(error);
     return nullptr;
   }
 
   return hashTable;
 }
 
+/* static */ void
+UPowerClient::GetDevicePropertiesCallback(DBusGProxy* aProxy,
+                                          DBusGProxyCall* aCall, void* aData)
+{
+  GError* error = nullptr;
+  GHashTable* hashTable = nullptr;
+  GType typeGHashTable = dbus_g_type_get_map("GHashTable", G_TYPE_STRING,
+                                             G_TYPE_VALUE);
+  if (!dbus_g_proxy_end_call(aProxy, aCall, &error, typeGHashTable,
+                             &hashTable, G_TYPE_INVALID)) {
+    g_printerr("Error: %s\n", error->message);
+    g_error_free(error);
+  } else {
+    sInstance->UpdateSavedInfo(hashTable);
+    hal::NotifyBatteryChange(hal::BatteryInformation(sInstance->mLevel,
+                                                     sInstance->mCharging,
+                                                     sInstance->mRemainingTime));
+    g_hash_table_unref(hashTable);
+  }
+}
+
+void
+UPowerClient::GetDevicePropertiesAsync(DBusGProxy* aProxy)
+{
+  dbus_g_proxy_begin_call(aProxy, "GetAll", GetDevicePropertiesCallback, nullptr,
+                          nullptr, G_TYPE_STRING,
+                          "org.freedesktop.UPower.Device", G_TYPE_INVALID);
+}
+
 void
 UPowerClient::UpdateSavedInfo(GHashTable* aHashTable)
 {
   bool isFull = false;
 
   /*
    * State values are confusing...
    * First of all, after looking at upower sources (0.9.13), it seems that