Demande d'avis : critique d'une classe toute simple

Version imprimable

Voir 40 message(s) de cette discussion en une page

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:

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:

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();
}

31/08/2009, 12h37
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.
31/08/2009, 13h33
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)
31/08/2009, 13h46
_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:

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é?

Citation:

Envoyé par _skip

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:

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

Et :

Code:

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:

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 =

31/08/2009, 14h33
Médinoc

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

Ben si, il applique au moins la RVO. :mrgreen: (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.
31/08/2009, 14h49
3DArchi
Salut,
Citation:
Envoyé par _skip

Code:

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

Stopwatch.cpp

Code:

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.
31/08/2009, 15h01
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:

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.
31/08/2009, 15h03
Arzar
Edit : Oups, grillé par Mac Lak :oops:.
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:

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; };
31/08/2009, 15h17
CedricMocquillon

Sinon, quitte à utiliser boost, autant utiliser boost::timer
31/08/2009, 17h49
_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

Citation:

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. :ccool:
31/08/2009, 18h09
Mac LAK

Citation:

Envoyé par _skip

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... ;)
31/08/2009, 20h48
_skip

Je connais déjà l'histoire de la lenteur des sorties consoles, ça existe dans tous les langages.

Même si mon expérience du C/C++ est jeune, pour l'instant on obtient d'excellents résultats avec le portage d'algos intensifs de java vers C++. Au départ il était question de limiter simplement l'overhead mémoire de l'algo le plus gourmand (clustering), puis au final les gains en performance constatés nous poussent à considérer le portage d'autres parties cruciales du système.

Ce sont des durées d'exécution pouvant passer les 45 min, dans lesquels certaines fonctions sont appelées plusieurs millions de fois. J'ai été surpris déjà à quel point des changements "simples" dans le code pouvaient faire gagner gros. De simples trucs comme stocker un itérateur de fin de collection dans une variable avant un gros for(), éviter les appels à vector.at(i) répétés, etc... Puis l'utilisation des threads aussi, éviter de locker au max, tout ça.

Pour l'instant je fais des mesures manuellement en utilisant l'outil immonde en ligne de commande (pas taper :aie:) fourni par mingw qui génère des pavés textes de 6mo dans lequel j'essaie de trier tant bien que mal. Je vais aussi avoir besoin d'analyser les fuites mais pour cela, je dois encore faire des recherches.
31/08/2009, 21h22
JolyLoic

Citation:

Envoyé par _skip

Même si mon expérience du C/C++ est jeune, pour l'instant on obtient d'excellents résultats avec le portage d'algos intensifs de java vers C++. Au départ il était question de limiter simplement l'overhead mémoire de l'algo le plus gourmand (clustering), puis au final les gains en performance constatés nous poussent à considérer le portage d'autres parties cruciales du système.

Pour ma culture personnelle, tu aurais un ordre de grandeur du gain obtenu ?
01/09/2009, 10h03
_skip

Il s'agit principalement de comparaisons et de multiplications de float sur des matrices éparses (je sais pas comment ça se dit en français "sparse").

Les principales opérations sont donc
- parcours de collection / listes / vectors
- recherche et ajout dans de grandes tables de hachage (map non ordonnées)

L'algo java écrit par moi-même avec optimisation intensive au profiler n'a pas battu son portage brut de décoffrage en c++ (écrit lui aussi par moi) qui était 30-40% plus rapide sur un temps d'exécution de 25 secondes environ.
Les optimisations réalisées plus tard (qui sont aussi conceptuelles et non seulement code ont permis de tomber autour des 8 secondes).

Niveau mémoire, la diminution est vraiment radicale aussi. Les gains sont principalement dus, je pense, au fait que nous évitons en c++ les opérations de boxings/unboxing qui interviennent des millions de fois dans l'algo java.

En utilisant des implémentations de collections de primitifs en java plutôt que le standard du framework, on aurait sans doute pu aussi pas mal gagner...
01/09/2009, 10h47
JolyLoic

Merci !

(pour info : sparse matrix = matrice creuse).
01/09/2009, 11h30
_skip

Merci à toi pour la traduction.
Enfin comme quoi, moralité de l'histoire, malgré les progrès des langages managés il existe encore certaines contraintes quant aux performances...
01/09/2009, 11h32
Mac LAK

Citation:

Envoyé par _skip

Enfin comme quoi, moralité de l'histoire, malgré les progrès des langages managés il existe encore certaines contraintes quant aux performances...

En doutais-tu réellement ? ;)
01/09/2009, 11h37
Médinoc

Apparemment, il s'agit surtout ici de problèmes de boxing liés aux collections. Problèmes qui sont absents des collections génériques en .Net, donc le problème n'est pas du aux langages managés en général, mais aux insuffisances sémantiques de java...

Voir 40 message(s) de cette discussion en une page