yarp::os::ThreadStats #1230

diegoferigo · 2017-05-23T17:17:34Z

Referring to #1207, these ThreadStats and MultipleThreadStats classes allow the computation of statistics on threads / periodic functions.

ThreadStats is API compatible with the current yarp::os::RateThreadCallbackAdapter class. I successfully validated and compared the methods. The main difference is the function I use to calculate the variance, but no significative differences emerged.

Beyond the possible usage inside RateThread, these classes might be very useful to debug issues like #1229, especially the MultipleThreadStats. I developed this class having in mind a clean and easy setup of many profiling points in a whole class codebase.

[WIP] I still have minor edits I'd like the last version will implements. For the time being, please let me know if you have any feedback about the current status.

(I'd prefer keeping this out of the next yarp master freeze for the time being)

lornat75 · 2017-05-26T10:34:46Z

src/libYARP_OS/include/yarp/os/ThreadStats.h

+    // Advanced statistics: histogram
+    // ==============================
+    unsigned bins_pitch_ms;
+    std::map<unsigned, unsigned> jitter_pdf;


Make sure this list is allocated before the thread starts running and deallocated after the thread stopped. If you modify the size of the list dynamically during the thread execution there may be a performance hit.

jitter_pdf entries are correctly allocated (twice, I just noticed) in _initializeJitterPDF() and _resetStats(). The dynamic allocation happens inside the map class, and since I didn't use any pointers, all the resources should be correctly deallocated when the thread is stopped.

drdanz · 2017-10-11T23:03:20Z

@diegoferigo Is this PR finished or abandoned? If it is finished, can you rebase it and remove the WIP from the title?

barbalberto · 2017-10-12T06:37:43Z

If I remember corectly, @diegoferigo was waiting for the clock refactoring to be completed before merging this PR to avoid too many comflicts.

The refactored clock is in devel from a couple of months now, so I think now is a good time to add this feature.
Please @diegoferigo take a look if something has to be updated due to the changes I did.

diegoferigo · 2017-10-13T08:13:47Z

@drdanz
The situation is exactly as @barbalberto explained. I'd like to test the RateThread code I have somewhere (after rebasing it on the new clock code) that uses this ThreadStats class instead of the current methods for statistics and push them together.

drdanz · 2017-10-13T16:35:01Z

Should we defer it to the next release then? We are releasing on the 23rd...

diegoferigo · 2017-10-25T15:06:20Z

I thought I had already replied to this message. Yes, I need further testing before hitting master.

Nicogene

After a quick glance it seems a nice patch!
I would add some doxygen documentation for the public API and a ThreadStatsTest to harness_os for testing the basic functionalities.
BTW nice work @diegoferigo 👍

traversaro · 2018-03-12T08:08:20Z

src/libYARP_OS/include/yarp/os/ThreadStats.h

+
+/*
+    Consider:
+    * |---------| Thread's or DUT's period


Device Under Testing (DUT)

traversaro · 2018-03-12T08:15:26Z

src/libYARP_OS/include/yarp/os/ThreadStats.h

+
+    // Constant values
+    // ===============
+    double m_window_length;       // Lenght of the window for calculating statistic features


diegoferigo · 2018-03-26T08:26:04Z

@drdanz Ready to go as soon as all checks succeed.

drdanz

A few other comments:

Each class should be in its own .h/.cpp file
You should be using the PIMPL idiom for the private members, see for example https://github.com/robotology/yarp/blob/c6cfb77a0c22ddd09add5a87f136ddddce1783af/src/libYARP_OS/include/yarp/os/Contact.h#L475:L479
I cannot really understand how to use the classes, can you replace them to the thread implementation? A couple of examples would be nice
Unit tests would be very appreciated

drdanz · 2018-03-29T16:24:46Z

src/libYARP_OS/include/yarp/os/ThreadStats.h

+namespace os {
+class ThreadStats;
+class MultipleThreadStats;
+const unsigned WINDOW_LENGTH_DEFAULT = 60;


These constants should not pollute the yarp::os namespace, perhaps they should be class members?
Also will it work with constexpr instead of const?

drdanz · 2018-03-29T16:29:27Z

src/libYARP_OS/src/ThreadStats.cpp

+ */
+
+#include <yarp/os/LockGuard.h>
+#include <yarp/os/ThreadStats.h>


ThreadStats.h should be the first include in the file ThreadStats.cpp. In this way you implicitly test that your .h file contains all includes and the definitions required

drdanz · 2018-03-29T17:45:46Z

doc/release/v3_0_0.md

@@ -62,6 +62,11 @@ Important Changes
 New Features
 ------------

+#### `YARP OS`


The YARP_OS section should be under the Libraries section

drdanz · 2018-03-29T17:49:29Z

src/libYARP_OS/include/yarp/os/ThreadStats.h

+ * BSD-3-Clause license. See the accompanying LICENSE file for details.
+ */
+
+#ifndef YARP_THREAD_STATS_H


This should be YARP_OS_THREADSTATS_H

This class implements and extends the statistics used by RateThreadCallbackAdapter

drdanz · 2018-10-03T12:55:32Z

@diegoferigo we should decide what to do with this PR. Honestly I haven't merged it yet because I cannot figure out how to use it, it would be great if you could rebase the PR and add at least some example...

drdanz · 2019-10-07T07:37:36Z

Closing for now, please reopen if you would like to add some docs or example.

diegoferigo · 2019-10-22T10:07:04Z

@drdanz Yes sorry but I didn't find enough time to clean this up.

Anyway, if we ever want to add profiling support to YARP, I would go towards solutions like Celtoys/Remotery. It is way more complete than what I implemented in this PR and it allows visualizing the report in the browser 😍

It is used under the hood by Ignition Gazebo. I really like how they implemented because is very compact and uses RAII to scope the profiled section.

Resources:

drdanz added the Status: In Progress label May 23, 2017

diegoferigo mentioned this pull request May 24, 2017

torque timeout while running wholeBodyDynamicsDevice throught yarprobotinterface robotology/icub-tech-support#449

Closed

diegoferigo force-pushed the ThreadStats branch from 294a252 to b90e834 Compare May 24, 2017 13:23

drdanz added Component: Library - YARP_os PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Target: YARP v2.3.72 labels May 25, 2017

lornat75 reviewed May 26, 2017

View reviewed changes

diegoferigo force-pushed the ThreadStats branch 2 times, most recently from e3ac81e to 48b9b85 Compare May 26, 2017 11:31

Nicogene added the PR Status: Changelog - Not OK This PR does not have a changelog entry, or the changelog needs to be fixed label Jun 21, 2017

drdanz added Target: YARP v2.3.74 and removed Target: YARP v2.3.72 labels Oct 16, 2017

drdanz added the PR Status: Rebase - Not OK This PR needs to be rebased over the latest commit in the target branch label Nov 27, 2017

diegoferigo requested review from aerydna, barbalberto, drdanz, Nicogene, pattacini, randaz81, traversaro and vtikha as code owners December 15, 2017 08:08

diegoferigo force-pushed the ThreadStats branch from 8e5e6ba to 9c24bc1 Compare December 15, 2017 08:08

Nicogene reviewed Dec 18, 2017

View reviewed changes

pattacini removed their request for review December 18, 2017 18:47

drdanz added the Target: Future label Feb 9, 2018

drdanz removed the Target: Future label Mar 12, 2018

drdanz added PR Status: Changelog - OK This PR has a proper changelog entry PR Status: Copyright - Not OK Files in this PR have copyright issues and removed PR Status: Changelog - Not OK This PR does not have a changelog entry, or the changelog needs to be fixed labels Mar 12, 2018

traversaro requested changes Mar 12, 2018

View reviewed changes

diegoferigo mentioned this pull request Mar 15, 2018

Thread and RateThread need a Network yarp object #1595

Closed

diegoferigo force-pushed the ThreadStats branch 2 times, most recently from cd7f875 to 78cd958 Compare March 26, 2018 08:20

diegoferigo force-pushed the ThreadStats branch from 78cd958 to 96bba7a Compare March 26, 2018 16:30

drdanz suggested changes Mar 29, 2018

View reviewed changes

diegoferigo added 3 commits May 17, 2018 21:03

ThreadStats: calculate statistics on periodic functions

67c374c

This class implements and extends the statistics used by RateThreadCallbackAdapter

MultipleThreadStats: container of many ThreadStats objects

2b98832

Changelog for ThreadStats and MultipleThreadStats new classes

a35511e

diegoferigo force-pushed the ThreadStats branch from 96bba7a to a35511e Compare May 17, 2018 19:10

aerydna added Target: Future and removed Target: YARP v3.0.0 labels May 24, 2018

drdanz added Resolution: No Feedback Received and removed Status: In Progress labels Oct 7, 2019

drdanz closed this Oct 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

yarp::os::ThreadStats #1230

yarp::os::ThreadStats #1230

diegoferigo commented May 23, 2017

lornat75 May 26, 2017

diegoferigo May 26, 2017 •

edited

Loading

drdanz commented Oct 11, 2017

barbalberto commented Oct 12, 2017

diegoferigo commented Oct 13, 2017

drdanz commented Oct 13, 2017

diegoferigo commented Oct 25, 2017 •

edited

Loading

Nicogene left a comment •

edited

Loading

traversaro Mar 12, 2018

traversaro Mar 12, 2018

diegoferigo commented Mar 26, 2018

drdanz left a comment

drdanz Mar 29, 2018

drdanz Mar 29, 2018

drdanz Mar 29, 2018

drdanz Mar 29, 2018

drdanz commented Oct 3, 2018

drdanz commented Oct 7, 2019

diegoferigo commented Oct 22, 2019

yarp::os::ThreadStats #1230

yarp::os::ThreadStats #1230

Conversation

diegoferigo commented May 23, 2017

lornat75 May 26, 2017

Choose a reason for hiding this comment

diegoferigo May 26, 2017 • edited Loading

Choose a reason for hiding this comment

drdanz commented Oct 11, 2017

barbalberto commented Oct 12, 2017

diegoferigo commented Oct 13, 2017

drdanz commented Oct 13, 2017

diegoferigo commented Oct 25, 2017 • edited Loading

Nicogene left a comment • edited Loading

Choose a reason for hiding this comment

traversaro Mar 12, 2018

Choose a reason for hiding this comment

traversaro Mar 12, 2018

Choose a reason for hiding this comment

diegoferigo commented Mar 26, 2018

drdanz left a comment

Choose a reason for hiding this comment

drdanz Mar 29, 2018

Choose a reason for hiding this comment

drdanz Mar 29, 2018

Choose a reason for hiding this comment

drdanz Mar 29, 2018

Choose a reason for hiding this comment

drdanz Mar 29, 2018

Choose a reason for hiding this comment

drdanz commented Oct 3, 2018

drdanz commented Oct 7, 2019

diegoferigo commented Oct 22, 2019

diegoferigo May 26, 2017 •

edited

Loading

diegoferigo commented Oct 25, 2017 •

edited

Loading

Nicogene left a comment •

edited

Loading