Changeset 10770 in vbox for trunk

Timestamp:

Jul 21, 2008 9:02:08 AM (16 years ago)

Author:

vboxsync

Message:

Main: Performance: Second m is redundant. Some locking-related todos.

Location:

trunk/src/VBox/Main

Files:

• PerformanceImpl.cpp (modified) (19 diffs)
• include/Performance.h (modified) (8 diffs)
• include/PerformanceImpl.h (modified) (2 diffs)

trunk/src/VBox/Main/PerformanceImpl.cpp

@@ -103 +103 @@
     /* @todo Obviously other platforms must be added as well. */
 #ifdef RT_OS_SOLARIS
-    m.mFactory = new pm::MetricFactorySolaris();
+    m.factory = new pm::MetricFactorySolaris();
 #endif
 #ifdef RT_OS_LINUX
-    m.mFactory = new pm::MetricFactoryLinux();
+    m.factory = new pm::MetricFactoryLinux();
 #endif
 #ifdef RT_OS_WINDOWS
-    m.mFactory = new pm::MetricFactoryWin();
+    m.factory = new pm::MetricFactoryWin();
 #endif
 #ifdef RT_OS_OS2
-    m.mFactory = new pm::MetricFactoryOS2();
+    m.factory = new pm::MetricFactoryOS2();
 #endif
 #ifdef RT_OS_DARWIN
-    m.mFactory = new pm::MetricFactoryDarwin();
+    m.factory = new pm::MetricFactoryDarwin();
 #endif
 
@@ -122 +122 @@
 
     /* Start resource usage sampler */
-    int vrc = RTTimerCreate (&m.mSampler, VBOX_USAGE_SAMPLER_MIN_INTERVAL,
+    int vrc = RTTimerCreate (&m.sampler, VBOX_USAGE_SAMPLER_MIN_INTERVAL,
                              &PerformanceCollector::staticSamplerCallback, this);
     AssertMsgRC (vrc, ("Failed to create resource usage "
@@ -158 +158 @@
 
     /* Destroy resource usage sampler */
-    int vrc = RTTimerDestroy (m.mSampler);
+    int vrc = RTTimerDestroy (m.sampler);
     AssertMsgRC (vrc, ("Failed to destroy resource usage "
                        "sampling timer (%Rra)\n", vrc));
-    m.mSampler = NULL;
-
-    delete m.mFactory;
-    m.mFactory = NULL;
+    m.sampler = NULL;
+
+    delete m.factory;
+    m.factory = NULL;
 
     LogFlowThisFuncLeave();
@@ -197 +197 @@
 ////////////////////////////////////////////////////////////////////////////////
 
-STDMETHODIMP PerformanceCollector::GetMetrics(ComSafeArrayIn(INPTR BSTR, metricNames),
-                                              ComSafeArrayIn(IUnknown *, objects),
-                                              ComSafeArrayOut(IPerformanceMetric *, outMetrics))
+STDMETHODIMP
+PerformanceCollector::GetMetrics (ComSafeArrayIn (INPTR BSTR, metricNames),
+                                  ComSafeArrayIn (IUnknown *, objects),
+                                  ComSafeArrayOut (IPerformanceMetric *, outMetrics))
 {
     //LogFlowThisFunc (("mState=%d, mType=%d\n", mState, mType));
@@ -208 +209 @@
     CheckComRCReturnRC (autoCaller.rc());
 
-    pm::Filter filter(ComSafeArrayInArg(metricNames), ComSafeArrayInArg(objects));
+    pm::Filter filter (ComSafeArrayInArg (metricNames),
+                       ComSafeArrayInArg (objects));
 
     AutoReadLock alock (this);
@@ -214 +216 @@
     MetricList filteredMetrics;
     MetricList::iterator it;
-    for (it = m.mMetrics.begin(); it != m.mMetrics.end(); ++it)
-        if (filter.match((*it)->getObject(), (*it)->getName()))
-            filteredMetrics.push_back(*it);
-
-    com::SafeIfaceArray<IPerformanceMetric> retMetrics(filteredMetrics.size());
+    for (it = m.metrics.begin(); it != m.metrics.end(); ++it)
+        if (filter.match ((*it)->getObject(), (*it)->getName()))
+            filteredMetrics.push_back (*it);
+
+    com::SafeIfaceArray<IPerformanceMetric> retMetrics (filteredMetrics.size());
     int i = 0;
     for (it = filteredMetrics.begin(); it != filteredMetrics.end(); ++it)
     {
-        ComObjPtr<PerformanceMetric> metric;
+        ComObjPtr <PerformanceMetric> metric;
         rc = metric.createObject();
         if (SUCCEEDED (rc))
             rc = metric->init (*it);
-        ComAssertComRCThrowRC (rc);
-        LogFlow(("PerformanceCollector::GetMetrics() store a metric at retMetrics[%d]...\n", i));
-        metric.queryInterfaceTo(&retMetrics[i++]);
+        AssertComRCReturnRC (rc);
+        LogFlow (("PerformanceCollector::GetMetrics() store a metric at "
+                  "retMetrics[%d]...\n", i));
+        metric.queryInterfaceTo (&retMetrics [++ i]);
     }
-    retMetrics.detachTo(ComSafeArrayOutArg(outMetrics));
+    retMetrics.detachTo (ComSafeArrayOutArg(outMetrics));
     return rc;
 }
 
-STDMETHODIMP PerformanceCollector::SetupMetrics (ComSafeArrayIn(INPTR BSTR, metricNames),
-                                                 ComSafeArrayIn(IUnknown *, objects),
-                                                 ULONG aPeriod, ULONG aCount)
+STDMETHODIMP
+PerformanceCollector::SetupMetrics (ComSafeArrayIn (INPTR BSTR, metricNames),
+                                    ComSafeArrayIn (IUnknown *, objects),
+                                    ULONG aPeriod, ULONG aCount)
 {
     AutoCaller autoCaller (this);
     CheckComRCReturnRC (autoCaller.rc());
 
-    pm::Filter filter(ComSafeArrayInArg(metricNames), ComSafeArrayInArg(objects));
+    pm::Filter filter (ComSafeArrayInArg (metricNames),
+                       ComSafeArrayInArg (objects));
 
     AutoWriteLock alock (this);
 
     BaseMetricList::iterator it;
-    for (it = m.mBaseMetrics.begin(); it != m.mBaseMetrics.end(); ++it)
+    for (it = m.baseMetrics.begin(); it != m.baseMetrics.end(); ++it)
         if (filter.match((*it)->getObject(), (*it)->getName()))
         {
@@ -256 +261 @@
 }
 
-STDMETHODIMP PerformanceCollector::EnableMetrics (ComSafeArrayIn(INPTR BSTR, metricNames),
-                                                  ComSafeArrayIn(IUnknown *, objects))
+STDMETHODIMP
+PerformanceCollector::EnableMetrics (ComSafeArrayIn (INPTR BSTR, metricNames),
+                                     ComSafeArrayIn (IUnknown *, objects))
 {
     AutoCaller autoCaller (this);
     CheckComRCReturnRC (autoCaller.rc());
 
-    pm::Filter filter(ComSafeArrayInArg(metricNames), ComSafeArrayInArg(objects));
+    pm::Filter filter (ComSafeArrayInArg (metricNames),
+                       ComSafeArrayInArg (objects));
+
+    /// @todo (r=dmik) why read lock below? individual elements get modified!
 
     AutoReadLock alock (this); /* Need a read lock to access mBaseMetrics */
 
     BaseMetricList::iterator it;
-    for (it = m.mBaseMetrics.begin(); it != m.mBaseMetrics.end(); ++it)
+    for (it = m.baseMetrics.begin(); it != m.baseMetrics.end(); ++it)
         if (filter.match((*it)->getObject(), (*it)->getName()))
             (*it)->enable();
@@ -274 +283 @@
 }
 
-STDMETHODIMP PerformanceCollector::DisableMetrics (ComSafeArrayIn(INPTR BSTR, metricNames),
-                                                   ComSafeArrayIn(IUnknown *, objects))
+STDMETHODIMP
+PerformanceCollector::DisableMetrics (ComSafeArrayIn (INPTR BSTR, metricNames),
+                                      ComSafeArrayIn (IUnknown *, objects))
 {
     AutoCaller autoCaller (this);
     CheckComRCReturnRC (autoCaller.rc());
 
-    pm::Filter filter(ComSafeArrayInArg(metricNames), ComSafeArrayInArg(objects));
+    pm::Filter filter (ComSafeArrayInArg (metricNames),
+                       ComSafeArrayInArg (objects));
+
+    /// @todo (r=dmik) why read lock below? individual elements get modified!
 
     AutoReadLock alock (this);
 
     BaseMetricList::iterator it;
-    for (it = m.mBaseMetrics.begin(); it != m.mBaseMetrics.end(); ++it)
+    for (it = m.baseMetrics.begin(); it != m.baseMetrics.end(); ++it)
         if (filter.match((*it)->getObject(), (*it)->getName()))
             (*it)->disable();
@@ -292 +305 @@
 }
 
-STDMETHODIMP PerformanceCollector::QueryMetricsData (ComSafeArrayIn(INPTR BSTR, metricNames),
-                                                     ComSafeArrayIn(IUnknown *, objects),
-                                                     ComSafeArrayOut(BSTR, outMetricNames),
-                                                     ComSafeArrayOut(IUnknown *, outObjects),
-                                                     ComSafeArrayOut(ULONG, outDataIndices),
-                                                     ComSafeArrayOut(ULONG, outDataLengths),
-                                                     ComSafeArrayOut(LONG, outData))
+STDMETHODIMP
+PerformanceCollector::QueryMetricsData (ComSafeArrayIn (INPTR BSTR, metricNames),
+                                        ComSafeArrayIn (IUnknown *, objects),
+                                        ComSafeArrayOut (BSTR, outMetricNames),
+                                        ComSafeArrayOut (IUnknown *, outObjects),
+                                        ComSafeArrayOut (ULONG, outDataIndices),
+                                        ComSafeArrayOut (ULONG, outDataLengths),
+                                        ComSafeArrayOut (LONG, outData))
 {
     AutoCaller autoCaller (this);
@@ -309 +323 @@
     /* Let's compute the size of the resulting flat array */
     size_t flatSize = 0, numberOfMetrics = 0;
-    for (it = m.mMetrics.begin(); it != m.mMetrics.end(); ++it)
+    for (it = m.metrics.begin(); it != m.metrics.end(); ++it)
     {
         /* @todo Filtering goes here! */
@@ -316 +330 @@
     }
     size_t flatIndex = 0;
-    com::SafeArray<BSTR> retNames(numberOfMetrics);
-    com::SafeIfaceArray<IUnknown> retObjects(numberOfMetrics);
-    com::SafeArray<ULONG> retIndices(numberOfMetrics);
-    com::SafeArray<ULONG> retLengths(numberOfMetrics);
-    com::SafeArray<LONG> retData(flatSize);
-
-    for (it = m.mMetrics.begin(), i = 0; it != m.mMetrics.end(); ++it)
+    com::SafeArray <BSTR> retNames (numberOfMetrics);
+    com::SafeIfaceArray <IUnknown> retObjects (numberOfMetrics);
+    com::SafeArray <ULONG> retIndices (numberOfMetrics);
+    com::SafeArray <ULONG> retLengths (numberOfMetrics);
+    com::SafeArray <LONG> retData (flatSize);
+
+    for (it = m.metrics.begin(), i = 0; it != m.metrics.end(); ++it)
     {
         /* @todo Filtering goes here! */
@@ -331 +345 @@
         Bstr tmp((*it)->getName());
         tmp.detachTo(&retNames[i]);
-        (*it)->getObject().queryInterfaceTo(&retObjects[i]);
+        (*it)->getObject().queryInterfaceTo (&retObjects[i]);
         retLengths[i] = length;
         retIndices[i] = flatIndex;
@@ -338 +352 @@
     }
 
-    retNames.detachTo(ComSafeArrayOutArg(outMetricNames));
-    retObjects.detachTo(ComSafeArrayOutArg(outObjects));
-    retIndices.detachTo(ComSafeArrayOutArg(outDataIndices));
-    retLengths.detachTo(ComSafeArrayOutArg(outDataLengths));
-    retData.detachTo(ComSafeArrayOutArg(outData));
+    retNames.detachTo (ComSafeArrayOutArg (outMetricNames));
+    retObjects.detachTo (ComSafeArrayOutArg (outObjects));
+    retIndices.detachTo (ComSafeArrayOutArg (outDataIndices));
+    retLengths.detachTo (ComSafeArrayOutArg (outDataLengths));
+    retData.detachTo (ComSafeArrayOutArg (outData));
     return S_OK;
 }
@@ -351 +365 @@
 void PerformanceCollector::registerBaseMetric (pm::BaseMetric *baseMetric)
 {
+    /// @todo (r=dmik) better always use AutoCaller unless you are 100% sure
+    /// the object is not uninitialized while you are doing something here
+
     AutoWriteLock alock (this);
-    m.mBaseMetrics.push_back (baseMetric);
+    m.baseMetrics.push_back (baseMetric);
 }
 
 void PerformanceCollector::registerMetric (pm::Metric *metric)
 {
+    /// @todo (r=dmik) better always use AutoCaller unless you are 100% sure
+    /// the object is not uninitialized while you are doing something here
+
     AutoWriteLock alock (this);
-    m.mMetrics.push_back (metric);
+    m.metrics.push_back (metric);
 }
 
 void PerformanceCollector::unregisterBaseMetricsFor (const ComPtr <IUnknown> &aObject)
 {
+    /// @todo (r=dmik) better always use AutoCaller unless you are 100% sure
+    /// the object is not uninitialized while you are doing something here
+
     AutoWriteLock alock (this);
-    std::remove_if (m.mBaseMetrics.begin(), m.mBaseMetrics.end(),
+    std::remove_if (m.baseMetrics.begin(), m.baseMetrics.end(),
                     std::bind2nd (std::mem_fun (&pm::BaseMetric::associatedWith),
                                   aObject));
@@ -371 +394 @@
 void PerformanceCollector::unregisterMetricsFor (const ComPtr <IUnknown> &aObject)
 {
+    /// @todo (r=dmik) better always use AutoCaller unless you are 100% sure
+    /// the object is not uninitialized while you are doing something here
+
     AutoWriteLock alock (this);
-    std::remove_if (m.mMetrics.begin(), m.mMetrics.end(),
+    std::remove_if (m.metrics.begin(), m.metrics.end(),
                     std::bind2nd (std::mem_fun (&pm::Metric::associatedWith),
                                   aObject));
@@ -386 +412 @@
     AssertReturnVoid (pvUser != NULL);
     PerformanceCollector *collector = static_cast <PerformanceCollector *> (pvUser);
-    Assert(collector->mMagic == MAGIC);
+    Assert (collector->mMagic == MAGIC);
     if (collector->mMagic == MAGIC)
     {
@@ -398 +424 @@
 
     uint64_t timestamp = RTTimeMilliTS();
-    std::for_each(m.mBaseMetrics.begin(), m.mBaseMetrics.end(),
-                  std::bind2nd (std::mem_fun (&pm::BaseMetric::collectorBeat), timestamp));
+    std::for_each (m.baseMetrics.begin(), m.baseMetrics.end(),
+                   std::bind2nd (std::mem_fun (&pm::BaseMetric::collectorBeat),
+                                 timestamp));
 }
 
@@ -446 +473 @@
 STDMETHODIMP PerformanceMetric::COMGETTER(MetricName) (BSTR *aMetricName)
 {
+    /// @todo (r=dmik) why do all these getters not do AutoCaller and
+    /// AutoReadLock? Is the underlying metric a constant object?
+
     Bstr tmp (mMetric->getName());
     tmp.detachTo (aMetricName);
@@ -471 +501 @@
 STDMETHODIMP PerformanceMetric::COMGETTER(Unit) (BSTR *aUnit)
 {
-    Bstr tmp(mMetric->getUnit());
+    Bstr tmp (mMetric->getUnit());
     tmp.detachTo(aUnit);
     return S_OK;

trunk/src/VBox/Main/include/Performance.h

@@ -29 +29 @@
 #include <string>
 
-namespace pm {
+namespace pm
+{
     /* CPU load is measured in 1/1000 of per cent. */
     const uint64_t PM_CPU_LOAD_MULTIPLIER = UINT64_C(100000);
@@ -109 +110 @@
         bool             mEnabled;
     };
-    
+
     class HostCpuLoad : public BaseMetric
     {
@@ -127 +128 @@
         SubMetric *mIdle;
     };
-    
+
     class HostCpuLoadRaw : public HostCpuLoad
     {
@@ -155 +156 @@
         SubMetric *mMHz;
     };
-    
+
     class HostRamUsage : public BaseMetric
     {
@@ -172 +173 @@
         SubMetric *mAvailable;
     };
-    
+
     class MachineCpuLoad : public BaseMetric
     {
@@ -189 +190 @@
         SubMetric *mKernel;
     };
-    
+
     class MachineCpuLoadRaw : public MachineCpuLoad
     {
@@ -261 +262 @@
             }
         }
-    
+
         ~Metric()
         {
@@ -276 +277 @@
         unsigned long getLength() { return mAggregate ? 1 : mBaseMetric->getLength(); };
         void query(unsigned long **data, unsigned long *count);
-    
+
     private:
         std::string mName;

trunk/src/VBox/Main/include/PerformanceImpl.h

@@ -229 +229 @@
     // public methods for internal purposes only
     // (ensure there is a caller and a read lock before calling them!)
-    //
-    pm::MetricFactory *getMetricFactory() { return m.mFactory; };
+
+    pm::MetricFactory *getMetricFactory() { return m.factory; };
 
     // for VirtualBoxSupportErrorInfoImpl
@@ -250 +250 @@
     unsigned int mMagic;
 
-    struct Data {
-        Data() : mFactory(0) {};
-
-        BaseMetricList     mBaseMetrics;
-        MetricList         mMetrics;
-        PRTTIMER           mSampler;
-        pm::MetricFactory *mFactory;
-    } m;
+    struct Data
+    {
+        Data() : factory(0) {};
+
+        BaseMetricList     baseMetrics;
+        MetricList         metrics;
+        PRTTIMER           sampler;
+        pm::MetricFactory *factory;
+    };
+
+    Data m;
 };