Petite question pour refactoring

**zeyr2mejetrem** · 14/06/2011, 14h07

Bonjour à tous,

J'ai une petite question existentielle concernant le refactoring d'une fonction (en général).
Quel est le code le plus optimisé selon vous ?

Voila le premier code

Code :

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

1
2
3
4
5
 
String unStringQuelconque = unObjet.getStringQuelconque();
if( (unStringQuelconque.equals("A")) || (unStringQuelconque.equals("B")) ){
 // Fait quelque chose de super intéressant :-)
}

Voila le deuxième code

Code :

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

1
2
3
4
 
if( (unObjet.getStringQuelconque().equals("A")) || (unObjet.getStringQuelconque().equals("B")) ){
 // Fait toujours quelque chose de super intéressant :-)
}

En gros ma question est l'instanciation d'une variable est elle préférable dans le cadre d'un traitement à la répétition d'un getter ?
Pourriez vous développer votre réponse en envisageant la conso mémoire et la rapidité d'exécution ? Merci d'avance

PS: Je sais bien que le code 2 est moins lisible

**tchize_** · 14/06/2011, 15h41

Tout dépend de ce que fait le getter. Si il est basique on peux le répéter. Par contre là, c'est surtout la lisibilité du code qui dois jouer. Donc:

Code :

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

1
2
3
4
String unStringQuelconque = unObjet.getStringQuelconque();
if( (unStringQuelconque.equals("A")) || (unStringQuelconque.equals("B")) ){
 // Fait quelque chose de super intéressant :-)
}

mais

Code :

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

1
2
3
if( (x.getY().equals("A")) || (x.getY().equals("B")) ){
 // Fait toujours quelque chose de super intéressant :-)
}

**zeyr2mejetrem** · 14/06/2011, 15h45

Ok. Cependant est on bien d'accord que la consommation mémoire dans le code A et dans le code B sont équivalentes ?

**tchize_** · 14/06/2011, 16h26

Ben ca dépend de ce qu'il y a dans le getter.

Code a va nécessiter un peu d'espace de stockage dans la Heap en plus pour la référence (64 bits à priori mais ce n'est pas standardisé la taille d'une référence en java)

**herve91** · 16/06/2011, 22h15

Juste pour être plus précis et ne pas introduire d'ambiguïté pour zeyr2mejetrem :

Envoyé par tchize_

Code a va nécessiter un peu d'espace de stockage dans la Stack

Sinon il est préférable d'écrire :

Code :

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

1
2
3
4
String s= unObjet.getStringQuelconque();
if ("A".equals(s) || "B".equals(s)){
 // Fait quelque chose de super intéressant :-)
}

pour pallier le cas ou s est null.

**tchize_** · 16/06/2011, 22h23

Tout à fait herve91, merci de la correction

**zeyr2mejetrem** · 16/06/2011, 22h51

Merci beaucoup pour vos réponses

**wax78** · 17/06/2011, 16h31

Envoyé par tchize_

Tout dépend de ce que fait le getter. Si il est basique on peux le répéter. Par contre là, c'est surtout la lisibilité du code qui dois jouer.

Je viens mettre mon grain de sel. Certe la lisibilité est importante. Mais un call (get) coute du temps.

Donc pourquoi le repeter ?

En effet et tu vas peut etre me contredire mais ce code ultra basique :

package Main;

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
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
public class bench
{
 
	int toto = 16;
 
	/**
         * @param args
         */
	public static void main(String[] args)
	{
		new bench();
 
	}
 
	public bench()
	{
		long start,stop,elapsed;
 
		start=System.currentTimeMillis();
		for (int i = 0; i < 500000; i++)
		{
			fct1();
		}
		stop=System.currentTimeMillis();
		elapsed=stop-start;
		System.err.println("ElapsedFCT1 =="+elapsed);
 
		start=System.currentTimeMillis();
		for (int i = 0; i < 500000; i++)
		{
			fct2();
		}
		stop=System.currentTimeMillis();
		elapsed=stop-start;
		System.err.println("ElapsedFCT2 =="+elapsed);
 
 
		start=System.currentTimeMillis();
		for (int i = 0; i < 500000; i++)
		{
			fct2();
		}
		stop=System.currentTimeMillis();
		elapsed=stop-start;
		System.err.println("ElapsedFCT2 =="+elapsed);
 
		start=System.currentTimeMillis();
		for (int i = 0; i < 500000; i++)
		{
			fct1();
		}
		stop=System.currentTimeMillis();
		elapsed=stop-start;
		System.err.println("ElapsedFCT1 =="+elapsed);
 
 
	}
	private boolean fct1()
	{
		int v = getToto();
		if ((v==15) || (v==17))
		return true;
		return false;
	}
	private boolean fct2()
	{
		if ((getToto()==15) || (getToto()==17))
		return true;
		return false;
	}
 
	public synchronized int getToto()
	{
		return toto;
	}
 
	public synchronized void setToto(int toto)
	{
		this.toto = toto;
	}
 
 
 
}

Qui est a peu pret du meme accabis montre que dans le cas ou l'on doit faire plusieur fois un get (donc un call au lieu d'acceder a la variable directement, ou de la mettre dans une autre variable que l'on rappelera plusieur fois dans le ou les if) il est preferable d'utiliser la FCT1, donc de faire 1 seul get dans une var que l'on compare apres, plutot que 2 fois les get.

Si je regarde le temps ecouler pour FCT1 et FCT2 :

ElapsedFCT1 ==17ms
ElapsedFCT2 ==29ms
ElapsedFCT2 ==29ms
ElapsedFCT1 ==15ms

Alors si j'en crois ce que je constate, un bon gros bloc genre

if (toto.getVariable()==qlqchsoe1) ||(toto.getVariable()==qlqchsoe2) ||(toto.getVariable()==qlqchsoe2) ||(toto.getVariable()==qlqchsoe3))
sera autant plus lent qu'il y'a aura de get (accumulation des temps de call j'imagine).

tandis que :

Object var = toto.getVariable;

if (var==qlqhchose1) || (var==qlqchose2) || (...)

Je me trompe ? (pour avoir dans certains programme (avec de bonne grosses boucles) supprimer des get pour acceder au valeur directement, j'ai vu des petits gains)

**zeyr2mejetrem** · 17/06/2011, 16h36

C'était bien mon sentiment premier.

D'un côté on alloue de la mémoire supplémentaire dans la stack et on gagne en vitesse d'exécution.
De l'autre on joue l'économie de mémoire.

Après le tout est de trouver un équillibre.

**wax78** · 17/06/2011, 16h38

Personnellement je n'avais même pas "conscience" du fait que ca allait augmenter le stack ^^ mais question rapidité ca oui pour l'avoir déjà essayer

Par contre "pour pallier le cas ou s est null." ca j'aime vraiment bien et je vais tenter de retenir pour eviter les test de null inutile avant mes if

**Rei Ichido** · 17/06/2011, 17h01

Juste sur ce dernier point, il vaut mieux que ça pète le plus tôt possible quand un objet est null alors qu'il ne devrait pas l'être ... Maintenant, si c'est normal d'avoir null, alors oui ça économise un test

Enfin, pour autant que la méthode equals soit conforme, ie renvoie false quand on lui passe null.

**tchize_** · 17/06/2011, 18h49

Alors oui je vais contredire.

Le test n'est pas représentatif.

1) Vous appelez une méthode synchronized, on est loin, très loin d'un "simple getter" comme je le mentionnais, les mécanismes de synchronisation étant très gourmands.

2) Le temps total du test est de moins de 1 seconde, on est largement en deçà du temps que met la jvm à mettre en place les mécanismes de compilation native. Avec une compilation native on "pourrait" (car on ne peut le prédire) se retrouver potentiellement à avoir ceci:
avec préstockage: N appels, la jvm décide que ça vaut pas le coup d'optimiser
sans: N*2 appels, la jvm décide qu'on appelle quand même souvent et faire de l'inlining de l'appel et de compiler nativement la méthode, on ne passe plus par la stack ni l’interpréteur.
Résultat: le double appel optimisé par le JIT est plus rapide que le simple appel qui a raté son optimisation.
conclusion: laissez la JVM faire ses optimisations.

Ensuite, le test montre un gain de +-40% sur une méthode qui ne fait presque rien d'autre que d'appeler d'autres méthode. Dans les cas concret, le reste du code a plus d'importance relative, ce qui amène à minimiser fortement le gain relatif. Comme vous allez le voir, le gain en termes de ms/opération est largement inférieur au gain que vous avez dans la compilation native.

Code modifié pour boucle indéfiniment.

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
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
public class Bench
{
 
        public static int LOOP_COUNT = 5000000;
 
	int toto = 16;
 
	/**
         * @param args
         */
	public static void main(String[] args)
	{
		while (true)
			new Bench();
 
	}
 
	public Bench()
	{
		long start,stop,elapsed;
 
		start=System.currentTimeMillis();
		for (int i = 0; i < LOOP_COUNT; i++)
		{
			fct1();
		}
		stop=System.currentTimeMillis();
		elapsed=stop-start;
		System.err.println("ElapsedFCT1 =="+elapsed);
 
		start=System.currentTimeMillis();
		for (int i = 0; i < LOOP_COUNT; i++)
		{
			fct2();
		}
		stop=System.currentTimeMillis();
		elapsed=stop-start;
		System.err.println("ElapsedFCT2 =="+elapsed);
 
 
		start=System.currentTimeMillis();
		for (int i = 0; i < LOOP_COUNT; i++)
		{
			fct2();
		}
		stop=System.currentTimeMillis();
		elapsed=stop-start;
		System.err.println("ElapsedFCT2 =="+elapsed);
 
		start=System.currentTimeMillis();
		for (int i = 0; i < LOOP_COUNT; i++)
		{
			fct1();
		}
		stop=System.currentTimeMillis();
		elapsed=stop-start;
		System.err.println("ElapsedFCT1 =="+elapsed);
 
 
	}
	private boolean fct1()
	{
		int v = getToto();
		if ((v==15) || (v==17))
		return true;
		return false;
	}
	private boolean fct2()
	{
		if ((getToto()==15) || (getToto()==17))
		return true;
		return false;
	}
 
	public synchronized int getToto()
	{
		return toto;
	}
 
	public synchronized void setToto(int toto)
	{
		this.toto = toto;
	}
 
 
 
}

Sortie:

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
ElapsedFCT1 ==239
ElapsedFCT2 ==477
ElapsedFCT2 ==515
ElapsedFCT1 ==252
ElapsedFCT1 ==252
ElapsedFCT2 ==515
ElapsedFCT2 ==507
ElapsedFCT1 ==249
ElapsedFCT1 ==227
ElapsedFCT2 ==506
ElapsedFCT2 ==506
ElapsedFCT1 ==253
ElapsedFCT1 ==17
ElapsedFCT2 ==55
ElapsedFCT2 ==55
ElapsedFCT1 ==45
ElapsedFCT1 ==18
ElapsedFCT2 ==56
ElapsedFCT2 ==54
ElapsedFCT1 ==45
ElapsedFCT1 ==18
ElapsedFCT2 ==56
ElapsedFCT2 ==56
ElapsedFCT1 ==43

On vois sur les premiers run qu'on gagne 1/2 de temps d'appel (similaire à vos résultats), mais on vois surtout qu'après quelques tours, la différence est bien autre. Le JIT, sans rien avoir besoin de faire dans le code, nous donne un gain de 9/10. Vous constaterez qu'il n'y a presque plus de différence notable entre le 4ème appel de la boucle (fct1)) et les deux appels de fct2.

Si je retire les synchronized (allez hop, on passe dans le cas simple getter), et que je multiplie par 10 le nombre d'appels (sinon c'est pas mesurables), j'obtiens ces résultats après stabilisation:

Code :

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

1
2
3
4
ElapsedFCT1 ==0
ElapsedFCT2 ==0
ElapsedFCT2 ==192
ElapsedFCT1 ==192

Plus aucune différence notable! On vois bien que les code compilé nativement par la jvm n'a plus de différence entre fct1 et fct2, par contre il y a une sacré différence entre le code compilé nativement et le reste.

Si demain vous avez un code critique et que vous pouvez démontrer que c'est le double appel qui plombe les perfs -> vous le changerez. Mais jusque là mon test démontre juste qu'on ne va rien gagner, pour un simple getter et les résultats de avec synchronized que je vais gagner entre 9 et 38ms tous les 5.000.000 appel (donc que je vais gagner une seconde d'exécution tous les entre 130 et 550 millions d'appel, suivant comment s'en sort le JIT

) Vous pensez vraiment que ça justifie, dans une méthode qui fait plein d'appels dans tous les sens de tout stocker en local, au risque de ne plus savoir quelle variable contient quoi?

Allez, un dernier pour la route, voilà ce que ça donne avec 3 fois fct2 et une fois fct1 dans la boucle:

Code :

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

1
2
3
4
ElapsedFCT2 ==0
ElapsedFCT2 ==0
ElapsedFCT2 ==192
ElapsedFCT1 ==197

Et oui, bien lu, fct1 est devenu le plus lent.

Moralité: pour ce genre de petite bidouille, vous ne pouvez absolument pas prévoir le comportement de la JVM, faites un code lisible et uniquement quand vous constaterez des lenteurs pour une application précise, commencez l'optimisation agressive! Optimisation prématurée est source de tous les maux en informatique

Et par rapport à 3,4,5 appels au même getter dans le if, mon conseil reste: optez pour ce qui est le plus lisible (et effectivement, avec 5 appels, une variable locale a tendance à devenir plus lisible

)