Discussion :

[_skip]

Bonjour,

Je viens de finir d'écrire une classe toute simple permettant de calculer le temps écoulé d'un point à l'autre d'un algo.
Vu que j'écris le plus souvent des routines en C++ pas vraiment orientées objet / réutilisables, j'aimerai savoir si dans ce code vous voyez des choses dérangeantes ou contraire aux bonnes pratiques.

Je pense aux points suivants :

-usage des inline
-retour de références sur membres par les getters (devrai-je préférer une copie? )
-affectation de classe avec "=" (dans ptime, il me semble que "=" n'est pas redéfini pour retourner une référence), faudrait-il y aller à coup de pointeurs?
-autres?

Merci d'avance...

Stopwatch.h

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
 
#ifndef _STOPWATCH_H
#define _STOPWATCH_H
 
#include <boost/date_time.hpp>
namespace bpt = boost::posix_time;
 
 
class Stopwatch
{
    public:
 
        //value returned in case of misuse of the class
        static const long NO_DURATION = -1000;
 
        Stopwatch( bool immediateStart  );
        virtual ~Stopwatch();
 
        //watch control
        inline void start();
        inline void stop();
        inline void reset();
 
        //result calculation
        long elapsedMillis() const;
 
        inline const bpt::ptime& getStartTime() const;
        inline const bpt::ptime& getStopTime() const;
 
    private:
 
        bpt::ptime startTime;
        bpt::ptime stopTime;
};
 
 
inline const bpt::ptime& Stopwatch::getStartTime() const
{
    return startTime;
}
 
inline const  bpt::ptime& Stopwatch::getStopTime() const
{
    return stopTime;
}
 
void inline Stopwatch::reset()
{
    startTime = bpt::not_a_date_time ;
    stopTime = bpt::not_a_date_time ;
}
 
 
void inline Stopwatch::start()
{
    startTime = bpt::microsec_clock::universal_time() ;
    stopTime = bpt::not_a_date_time ;
}
 
void inline Stopwatch::stop()
{
    stopTime = bpt::microsec_clock::universal_time() ;
}
 
#endif	/* _STOPWATCH_H */

Stopwatch.cpp

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
 
 
#include "Stopwatch.h"
 
 
 
Stopwatch::Stopwatch( bool immediateStart )
: startTime( immediateStart ? bpt::microsec_clock::universal_time() : bpt::not_a_date_time )
{
}
 
Stopwatch::~Stopwatch()
{
}
 
long Stopwatch::elapsedMillis() const
{
    //watch isn't running...
    if( startTime.is_not_a_date_time() )
        return NO_DURATION;
 
    //stop() wasn't called, return approx so far;
    if(  stopTime.is_not_a_date_time() )
        return (bpt::microsec_clock::universal_time() - startTime).total_milliseconds();
 
    //normal
    return (stopTime - startTime).total_milliseconds();
}

[Médinoc]

• Usage des inline: Ici, je pense que tes accesseurs inline seraient plus lisibles en étant directement dans la classe.
• Ça dépend de ce qu'est exactement un boost::posix_time::ptime. Si c'est un truc assez petit, un retour par copie serait mieux.
• Pour les = : Je ne comprends pas ce que tu dis. Tes variables membres elles-mêmes ne sont pas des références, donc aucun problème.
• Autres:
  
  • Ton constructeur devrait être déclaré explicit.

[Arzar]

Autre chose, le destructeur virtuel ne sert à rien ici. Autant le supprimer du code et laisser le compilateur générer lui-même un destructeur par défaut.
(pour rappel : destructeur virtuel <=> héritage)

[_skip]

Merci beaucoup, la copie avec "=" c'est un point qui me pose certains problèmes de compréhension.

Pour :
startTime = bpt::microsec_clock::universal_time() ;

Sachant que
bpt::microsec_clock::universal_time() retourne un ptime;

combien de copie cela provoque-t-il ? (si on ne considère pas l'optimisation des valeurs de retours. ), Une seule ou bien 2?

Admettons par exemple que

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
Object  maFonction()
{
    Object o(....);
    return o;
}

Sans avoir jamais pu être sûr :

Cas 1:
Object p;
p = maFonction(); //2 copies, 1 pour return dans maFonction, 1 pour p?

Cas 2:
Object p( maFonction() ); //1 ou 2 copies???

Cas 3:
Object& p = maFonction(); //1 copie.

En supposant que le retour de maFonction ne puisse pas être optimisé?

[Arzar]

Merci beaucoup, la copie avec "=" c'est un point qui me pose certains problèmes de compréhension.

Et pour cause, c'est assez subtil.
Un splendide article sur l'élision de copie pour éclaircir le tout : ici

En résumé :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
 
Classe a;
Classe b = a; // constructeur par copie
Classe c;
c = b; // opérateur =

Et :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
 
Classe foo()
{
   Classe c; // l'objet local a un nom, 
                //d'ou "Named" Return Value Optimization
   return c;
}
 
Classe foo2()
{
   return Classe(); // pas de nom d'ou Return Value Optimization
}
 
Classe a = foo(); // NRVO : constructeur par défaut
Classe b = foo2() // RVO : constructeur par défaut
a = foo(); // NRVO : constructeur par défaut + opérateur =
b = foo2(); // RVO : constructeur par défaut + opérateur =

Seule exception connue : MSVC n'applique (bizarement) pas la NRVO en debug, ce qui donne :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
 
Classe a = foo(); // constucteur par défaut, constructeur par copie
Classe b = foo2(); // RVO : constructeur par défaut
a = foo(); // constructeur par défaut, constructeur par copie, opérateur =
b = foo2(); // RVO : constructeur par défaut, opérateur =

[Médinoc]

@Arzar: À ma connaissance, Visual n'applique aucune optimisation en Debug. Pas même l'inlining d'une fonction déclarée __forceinline.

[Arzar]

Ben si, il applique au moins la RVO. (mais pas la NRVO ce qui est un peu idiot)

Bon _skip, j'ai la flemme de refaire le même schéma quand une fonction renvoie une lvalue, (comme par exemple un accesseur d'une classe renvoyant un membre) mais ça revient à peu près au même. En gros :

Retour par valeur = une copie. (pas deux).
Retour par référence = pas de copie.

Sachant que pour des petits objets comme des int ou (je suppose) des bpt::ptime, la copie est tout aussi rapide que le renvoie par référence, bah autant renvoyer par valeur.

[Mac LAK]

Déjà, même si j'enfonce les portes ouvertes, il n'y a qu'UN SEUL point critique à vérifier dans une telle classe : le temps pris par l'acquisition du timestamp courant.

Le reste, comme l'initialisation, le calcul du delta et tout et tout, c'est hors du code chronométré, donc on se contrefout du temps que ça prend. Cela fait partie de l'overhead général du profiling, certes, mais cela ne constitue pas un problème en soi pour la mesure de performances elle-même.

De plus, l'acquisition du temps de départ doit être la dernière instruction de la séquence "Start", et la première dans la séquence "Stop".

Donc, déjà :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
void inline Stopwatch::start()
{
    // J'ai permuté les deux lignes.
    stopTime = bpt::not_a_date_time ;
    startTime = bpt::microsec_clock::universal_time() ;
}

Ensuite, qu'est-ce que tu t'embêtes avec des accesseurs là-dedans ? Tu as besoin de peu de méthodes pour une telle classe :

• Une initialisation (et encore...).
• Une méthode "Start" (qui effectue un "reset" tacite AVANT de lancer le chrono).
• Une méthode "Stop".
• Une méthode "GetDelta", qui renvoie ce qui te convient le mieux (double représentant des secondes, texte lisible, entier long en nanosecondes, tout dépend de tes besoins).

En dehors de ça, tout le reste, c'est de la littérature et, surtout, de la perte de temps.

Je te déconseille également très fortement de démarrer ton chrono lors de la création de la classe : tu n'as aucune maîtrise sur le post-traitement une fois sorti du "}" du constructeur, et donc le temps que cela pourrait prendre.

Bannir aussi les appels intermédiaires : tout appel aux fonctions de chrono pendant le chronométrage est, potentiellement, un vrai gouffre suivant la plate-forme !


Dernier point, qui est en fait le détail du premier que je donnais (le temps pris par l'acquisition du timestamp) : utiliser une fonction portable pour récupérer le chrono courant, en soi, ce n'est pas forcément une mauvaise idée... Sauf qu'elle est mauvaise la plupart du temps ! J'explique : les fonctions les plus rapides, ce seront bien entendu celles de l'OS directement, c'est une évidence. Or, le format utilisé par exemple sous Windows et sous Linux n'ont RIEN en commun !

Sous Windows :

• GetTickCount, précision à la milliseconde, renvoie un entier 32 bits.
• QueryPerformanceCounter, précision souvent inférieure à la NANOseconde, renvoie un entier 64 bits.

Sous Linux :

• gettimeofday, précision à la microseconde, renvoie une structure (souvent de 2 x 32 bits).
• time, précision à la seconde, renvoie un time_t (souvent un entier 32 bits).

Bref, rien à voir, donc... Les conversions inhérentes à un chronomètre portable peuvent poser des problèmes non négligeables de précision sur des chronométrages courts. Il faut aussi savoir combien de temps prennent les appels à ces fonctions, bien entendu, et pour ceci tu n'as guère d'autre choix que de les bencher. Disons un million d'appels en boucle au minimum, ou tout chiffre suffisant pour avoir au moins une dizaine de secondes d'exécution pour ta boucle afin d'avoir une moyenne statistique du temps d'exécution à peu près correct. Pense à mettre le processus en priorité très haute, bien sûr, et à ne pas avoir d'autres processus actifs (inclus l'environnement de développement).
N'oublie pas qu'un appel système, potentiellement, permet au scheduler de reprendre la main et de basculer sur un autre thread/processus !! Et que les fonctions de date/heure sont, justement, des fonctions système...

A ta place, je passerais sur une classe définie ainsi :

• Codée sous forme de template pour assurer un inlining maximal.
• Gérant un facteur de précision, calculé soit dans le constructeur, soit par compilation conditionnelle : ceci te permet d'être portable et de t'adapter à la précision disponible sur ta plate-forme.
  Cette précision n'intervient que dans le calcul du delta, donc n'impacte pas le chronométrage lui-même.
• Prévue pour au moins trois cas : Windows, Linux et "autres" (=> dans ce cas, utiliser boost ou les fonctions C standard).
  Pour ceci, compilation conditionnelle et/ou spécialisation de template via la CC (tu instancies un template qui, en fait, est déjà un template spécialisé en fonction de la plate-forme).
• Pas besoin de gestion d'erreurs lourde, c'est inutile. Mais si tu dois en faire, c'est impérativement dans la fonction de calcul du delta ou le constructeur, et nulle part ailleurs. Les méthodes "Start" et "Stop" doivent rester de simples appels à la fonction de récupération du timestamp.

En espérant avoir pu t'aider.

[3DArchi]

Salut,

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
 
        //value returned in case of misuse of the class
        static const long NO_DURATION = -1000;

Stopwatch.cpp

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
 
long Stopwatch::elapsedMillis() const
{
    //watch isn't running...
    if( startTime.is_not_a_date_time() )
        return NO_DURATION;

Personnellement, je n'aime pas les valeurs 'particulières' retournées par une fonction indiquant un état d'erreur. Je vois à priori 3 solutions :
1/ Programmation par contrat : le contrat de ta classe spécifie qu'une pré-condition à l'appel de elapsedMillis() est l'appel à start(). Ta classe n'a pas à tester startTime.is_not_a_date_time() car c'est une rupture du contrat de faire l'appel à elapsedMillis() sans avoir appeler start(). En rompant le contrat, elle n'est pas tenue d'avoir une réponse valide. Le respect de la précondition du contrat est de la responsabilité de l'appelant. A lui de veiller que start a bien été appelé.
2/ Lever une exception dans elapsedMillis() si start() n'a pas été appelé plutôt que de renvoyer une valeur 'invalide'.
3/ Utiliser Boost.Optional si vraiment tu veux toujours avoir un retour.

[Arzar]

Edit : Oups, grillé par Mac Lak .
Ben +1 avec lui.

Peut-être est-il possible aussi de simplifier un peu le design ? Est-ce que les fonction reset(), getStartTime() et getStopTime() ne sont pas un peu superflues ?

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
 
class Timer
{
   Timer(); // initialize start_time
   Start(); // start_time = now()
   Elapsed(); // return now() - start_time
 
private:
   boost::ptime start_time;
};

[CedricMocquillon]

Sinon, quitte à utiliser boost, autant utiliser boost::timer

[_skip]

Merci à tous!

Suite aux réactions, j'ai fait les choses suivantes :

- Les fonctions getX() renvoient des copies, elles ne sont pas utilisées dans un contexte critique et ptime est de taille très modérée.
- Le destructeur virtuel vide a été retiré.
- Le démarrage au constructeur a été retiré, il faut dorénavant un appel à start() dans tous les cas, seul un constructeur sans paramètre subsiste.
- Ordre des appels dans start modifié pour affecter startTime en dernier.

Pour le reste,
Personne n'a parlé d'un usage abusif des inline, donc j'ose supposer que c'était pas complètement idiot pour ces petites méthodes même si à en croire les ressources trouvées concernant GCC, il y a de bonnes chances qu'une compilation O3 fasse tout de même cette opération.

Par ailleurs, j'ai commencé par utiliser une soustraction de std:.clock() et effectivement je n'avais pas la même échelle sur linux et sur windows, c'est ce qui a motivé cette classe.

J'ai en effet pas vu qu'il existait un timer dans boost, pour rejoindre la discussion, l'auteur met en garde contre la précision éventuelle de sa classe, il y a aussi ce bout de texte à la fin :

http://www.boost.org/doc/libs/1_40_0...imer/timer.htm

There have been several requests for platform specific implementations to use supposedly high-performance timers from the operating system API. John Maddock submitted an implementation using the Win32 API. Tests showed that while the precision of these timers was high, the latency was sometimes very much higher than for the std::clock() function, and that is very bad. Furthermore, results using the Win32 API were very dependent on both the compiler (Microsoft and Borland were tested) and the operating system version (Windows NT, Windows 95, etc.) Thus the std::clock() function was much more reliable, and so was retained even on this platform with its own timer API.

Ce n'est clairement pas un problème super simple. Toutefois cette classe ne sera pas utilisée pour mesurer des temps d'exécution ultra courts. Son but était surtout d'afficher une durée dans la console à la fin d'un traitement. Celui-ci prend entre 400ms et 45min.
Donc je suis ravi des conseils de Mac Lak (notamment) sur la prise en compte de ce problème et le calcul de l'overhead, mais je ne pense pas aller si loin, tout en sachant que ce sera pas l'idéal dans les chronométrages très courts.

Je suis aussi très content d'avoir de la lecture concernant la RVO, je sais que ça existe parce c'est mentionné dans le Scott Meyers, toutefois compter là dessus me semblait présenté comme une solution de dernier recours lorsque retourner un objet du tas était trop dangereux, à voir ce n'est pas vraiment le cas.
Pour d'autres parties de mon code, notamment des méthodes retournant des grosses collections, j'ai choisi de retourner des shared_ptr pour ne pas avoir de doute... Ai-je eu tort? Je ne sais plus trop... Faut que j'épluche cet article.
Je suis plus ou moins toujours parti de l'idée de ne pas laisser l'appelant se débrouiller avec un objet à deleter.

Pour 3DArchi, je pourrai en effet lever une exception, j'ai vu l'article sur boost::optionale, ca me servira sans doute dans un autre cas...

En tout cas merci à tous pour ces avis très constructifs, c'est un plaisir.

[Mac LAK]

Ce n'est clairement pas un problème super simple. Toutefois cette classe ne sera pas utilisée pour mesurer des temps d'exécution ultra courts. Son but était surtout d'afficher une durée dans la console à la fin d'un traitement. Celui-ci prend entre 400ms et 45min.
Donc je suis ravi des conseils de Mac Lak (notamment) sur la prise en compte de ce problème et le calcul de l'overhead, mais je ne pense pas aller si loin, tout en sachant que ce sera pas l'idéal dans les chronométrages très courts.

De rien. Mais sinon, en effet, inutile de te prendre trop fort la tête pour des durées aussi "longues" (à l'échelle du CPU du moins) que presque une demi-seconde...
Les optimisations que tu as déjà faites suffiront amplement, je pense, même si un passage par un template (pour l'inlining) ne serait pas forcément néfaste en soi, mais plus par principe en fait.
Les divers points critiques que je donne dans mon post précédent sont surtout nécessaires quand on commence à mesurer des temps inférieurs au quantum de temps de l'OS, c'est à dire classiquement 10 à 20 ms. J'avoue avoir été "trompé" par le fait que tu utilisais un timestamp descendant à la microseconde en terme de résolution, donc souvent utilisé pour des chronométrages précis.

Toutefois, à titre d'information personnelle (et de vérification de fiabilité), à ta place je ferais quand même un bench de mes fonctions de récupération de timestamp sur mes diverses plate-formes, afin de m'assurer qu'elles ne sont pas implémentées n'importe comment. Ce n'est pas long à faire, et ça permet de vérifier que ton compilateur et le portage de ta librairie (STL ou Boost) est correct sur cette fonction précise, tout en te donnant une vague idée de l'overhead des chronométrages. Au pire, ça te servira toujours si jamais tu dois faire des chronos précis, au mieux ça enrichira tes connaissances système.

Pour l'anecdote, si tu affiches les résultats des chronos sur la console : pense un jour à chronométrer une instruction "cout <<", ou "printf", pour te faire un coup de frayeur sur le temps que ça peut bouffer...