Gestion de la mémoire - Memory leak
Bonjour à tous,
Je viens vers vous parce que cela fait quelques jours que je m'arrache les cheveux sur un problème. Le projet sur lequel je travaille (dans ma boite) a été codé il y a quelques années, et est donc sans ARC et tout ce qui nous simplifie la vie quant à la gestion de la mémoire.
C'est du code de boucher, et c'est moi qui doit "m'amuser" à le maintenir...
J'suis une assez grosse tourte en gestion de la mémoire, j'imagine qu'une solution doit être devant mon nez mais là , je n'ai plus d'idées...
Tout d'abord ma première question, est-ce que faire un substring d'un NSString, créé un nouvel espace en mémoire qu'il faudra donc releaser au bout d'un moment ?
- (BOOL) isString
{
// Search the first double quoted block after index
NSRange r = [[source substringFromIndex:index] rangeOfString:patternDoublequotedString options:NSRegularExpressionSearch];
if ( r.location == NSNotFound )
return NO;
SwingInteger length = r.length;
// Get the string without double quote
NSString *stringValue = [source substringWithRange:NSMakeRange(index +1, length - 2)];
// Save it
// tokenValue => NSString
tokenValue = stringValue;
index += length;
return YES;
}
Cette fonction par exemple, (si j'ai bien compris) me créé des espaces mémoires qui ne sont pas releasés.
(voir capture d'écran Instruments)
Cette fonction est appelée dans une boucle while pour interpréter un script par exemple. Et fatalement, lorsque le script est très grand, ça me donne des memory warning et l'application finit par crasher.
D'autres fonctions du même acabit sont appelées dans cette boucle while, et sont également repérées dans instruments (voir capture n°2).
Je ne donne volontairement pas plus d'extrait de code parce que j'aimerais déjà comprendre si cette petite partie ici, est safe au niveau de la gestion de la mémoire. Je pourrais un peu plus détailler si nécessaire.
Je vous remercie d'avance de toute l'aide que vous pourrez m'apporter !
MoKeS
Réponses
Toute méthode dont le nom ne commence pas par "alloc", "new" ou "copy" est sensée renvoyer un objet autoreleasé.
Donc, oui une nouvelle instance est bien créée, et aussitôt ajoutée à l'autorelease pool courant.
L'autorelease pool est vidé à la fin de la run loop, c'est à dire quand tout le code a répondu aux événements reçus, et que les vues ont été rafraichies.
Maintenant, la solution la plus évidente est de créer un NSAutoreleasePool juste avant le traitement et de le détruire juste après. Il faudra peut-être le drainer de temps en temps pour éviter ce problème.
@Alf1996 :
Merci de ta réponse. Je confirme, c'est un très gros projet. J'ai déjà essayé de convertir tout le projet en ARC, mais certaines parties sont tellement pointilleuses et compliquées (voit ce mot comme "c'est pas moi qui ai fait ce code, il n'est pas documenté) que le projet au final ne fonctionnait plus du tout... De plus, cette application sera amenée à disparaà®tre étant donné qu'une nouvelle from scratch va être faite (par moi sans doute si je ne me suis pas barré d'ici là ). En attendant, je dois juste résoudre les "petits bugs" du genre et celui ci fait planter l'application d'un client, et c'est pour ça que je dois mettre les mains dans le cambouis...
@Céroce :
Merci pour ta réponse. C'est bien ce que je pensais, que NORMALEMENT cela doit être (auto)releasé, or si tu regardes les deux captures d'écran que j'ai faite, ça n'a pas l'air d'être le cas et c'est cela même que je n'arrive pas à comprendre...
Si je devais rapidement schématiser comment est gaulé cette partie ça serait comme cela :
Le problème ici se situe donc dans la "fonction 2", c'est là où plein de malloc sont fait, et jamais releasé, ce qui sature la mémoire (voir capture d'écran n°2 du premier post).
Alors j'ai déjà essayé de mettre des autorelease pool dans la "fonction 2", ou même directement dans la fonction qui génère ces malloc, mais du coup j'ai des EXC_BAD_ACCESS dans "pool". Encore une fois, la plupart de ce code ce n'est pas moi qui l'ai implémenté, si j'enlève un de ces NSAutoreleasePool, ça fait planter le reste à coup de EXC_BAD_ACCESS.
Alors tokenvalue est une variable globale de la classe définie dans le .h.
Un getter a été mis en place pour pouvoir récupérer cette variable. Dans ce cas, c'est juste une fonction qui fait un :
return tokenValue;
Et donc là on écrase l'ancien tokenValue en direct (sans accesseur) avec un objet stringValue créé avec un constructeur de commodité (donc autorelease par définition). En fait de sauvegarde, ce code ne fait qu'écraser le pointeur et non sauvegarder l'objet. Dans ce cas, pas étonnant que ça tienne pas debout.
La bonne écriture serait plutôt
Et on oublie pas de faire le release de tokenValue dans le dealloc de la classe bien sûr...
Bon après ce n'est pas dit que ce soit le seul problème dans ce code que tu as récupéré mais déjà ça ne pourra qu'améliorer les choses.
Et du coup à ce stade plutôt que d'utiliser une ivar et de coder soit même l'appel à "retain" et "release" et s'assurer qu'on fait les choses correctement et tout... autant utiliser directement une @property(retain) qui sont là pour ça et prennent en charge tous ces aspects de gestion mémoire pour toi !
En lisant "variable globale de la classe", j'ai pensé à une variable d'instance mal exprimée mais effectivement.
Je cautionne pas trop cette approche lorsqu'on manipule une instance de la classe qu'on code. C'est tout de même le minimum de savoir ce qu'on fait à ce niveau. Mais bon, c'est un choix qui se défend mais c'est pénalisant au niveau perfs.
Avant ARC éventuellement un peu effectivement... mais les quelques nanosecondes perdues par l'appel d'un setter " appel bien souvent inliné par le compilateur d'ailleurs "valent largement le coup vu tout ce qu'apportent les properties de toutes façons " du KVO à la gestion mémoire en passant par le thread-safety surtout " et évitent bien des soucis là où le code "manuel" que tu proposes ne les empêche pas (cas d'égalité des pointeurs ancien et nouveau, thread-safety etc).
Entre d'essayer à une optimisation qui va te faire gagner quelques ridicules nanosecondes mais ne pas être assez secure... et utiliser une @property et laisser le compilateur faire un boulot d'optimisation qu'il sait bien mieux faire que toi et avec une meilleure optimisation (entre autres atomique, thread-safe, et j'en passe), mon choix est vite fait.
De plus on parle ici à une personne qui visiblement n'est pas à l'aise avec toutes ces subtilités de gestion mémoire et donc je suppose encore moins avec les notions de thread-safety et d'accès atomiques (pour preuve ça ne l'a pas choquée plus que cela d'avoir une variable globale utilisée ainsi c'est dire !!!) donc le but ici est loin de gagner quelques ridicules nanosecondes mais plutôt d'utiliser un code efficace et qui est secure et s'occupe de tout, autrement dit le code que génère LLVM bien mieux que si on le faisait nous même (cf les articles de Mike Ash sur le sujet et sur les implémentations et optimisations faites par LLVM quand il génère le code assembleur)
Et enfin le jour où en + tu passes à ARC où tout cela est encore + vrai (car la balance ses retain/release est optimisée à la compilation encore + que si tu faisais des autorelease toi même (voir les vidéos WWDC sur le sujet)) il n'y a vraiment plus à hésiter.
Je pense que j'ai du faire une référence à une variable d'instance mal exprimée. Je suis un peu maladroit sur les termes, et en voyant AliGator parler de NSUserDefault j'ai compris mon erreur. (C'est pour ça que dans un premier temps j'avais dit "variable globale" dans le .h)
Ici par exemple dans mon .h j'ai :
(Y'a des Swing partout parce qu'ils ont décidé de renommer tous les types ... no comment)
Quoi qu'il en soit, j'ai bien lu ce que vous avez écrit et cela me semble plus clair. Donc j'ai créé une petite fonction :
J'ai remplacé chaque "token value = " dans mon code par cette fonction.
Malheureusement j'ai l'impression que ce n'est toujours pas bon, toujours un Memory Warning suivit d'un crash de l'application. J'ai de nouveau lancé instruments et c'est toujours situé au même endroit, je remets une capture d'écran ! http://hpics.li/bd20be6
En théorie "stringToSave" doit être release par l'autorelease pool non ?
Ton crash n'est pas dû à un memory leaks mais plutôt à une trop grande consommation de mémoire. Tu vas devoir bien comprendre le code pour savoir où placer des autoreleasepool.
Si tu te trompes, des objets qui ont été libérés par des autorelease pool, continueront à être utilisé et tu auras des EXEC_BAD_ACCESS.
(activer les NSZombie pour savoir lesquels).
La version longue:
Oui il est bien dans l'autorelease pool, mais comme tu fais un "retain", il ne sera pas libéré par l'autorelease pool, tu en pris la responsabilité, c'est donc à toi d'appeler release dessus quand tu n'en auras plus besoin.
Il faut que tu relises un doc sur la gestion mémoire et le reference counting.
En deux mots, le principe de base est simple : les objets sont libérés quand leur retainCount atteint 0.
Quand un objet est créé son retainCount est de 1.
A chaque fois que tu appelles retain sur l'objet son retainCount est incrémenté.
A chaque fois que tu appelles release son retainCount est décrémenté.
Quand tu appelles autorelease sur un objet tu différes à plus tard l'appel du message release.
"plus tard" signifiant quand l'autorelease pool courante sera vidé.
Après pour savoir où on en est avec les objets qu'on a obtenu à partir des API, il faut connaà®tre les conventions.
Tous les méthodes dont les noms commencent par alloc, new, copy retournent un objet dont la responsabilité incombent à l'appelant, c'est-à -dire que tu devras appeler release ou autorelease toi même sur cet objet, toutes les autres renvoient un objet qui a déjà reçu un message autorelease.
Une fois que tu as compris cela, tout le reste en découle par logique, parfois la logique est complexe, mais dans ton cas, je ne pense pas.
Pour du code "normal", tu n'as pas trop à te soucier des objets qui sont dans l'autorelease pool, ils seront libérés à la fin du cycle de courant, comme l'a expliqué Ali, par l'autorelease pool courante qui a été créée pour toi.
Par contre, si tu alloues beaucoup d'objets pendant un cycle, alors tu risques de saturer la mémoire avant que l'autorelease pool n'ait été vidée. C'est le cas quand tu fais beaucoup de manipulation de NSStrings sur des grosses strings par exemple. Chaque appel à substringWithRange: par exemple va créer une nouvelle string qui est placée dans l'autorelease pool.
Dans ce cas, ARC ou pas ARC, c'est à toi de créer des autorelease pool à des endroits stratégiques pour libérer un peu de mémoire quand c'est possible.
Généralement on mets l'autorelease pool dans un while ou un for, pour vider tous les strings intermédiaires qui ont été créés pendant une itération de la boucle.
Pour que cela fonctionne, il faut qu'aucun objet temporaire ne soit utilisé en dehors de l'autoreleasepool, par exemple le code suivant va faire un EXEC_BAD_ACCESS
J'ai l'impression que c'est le genre de problème que tu as avec ton token en variable d'instance.
Ce n'est jamais très bon d'avoir une variable qui a un rôle temporaire en variable d'instance (ou en variable globale).
Parfois on n'a pas le choix, mais dans ce cas il faut être très vigilant, le mieux étant de prendre la responsabilité de la variable pour bien comprendre à quel moment la mémoire est libérée:
Quand le code est complexe, tu peux te servir de la valeur nil pour savoir si tu as déjà libérée la variable ou pas.
La mauvaise nouvelle, c'est que tu vas devoir bien comprendre le code auquel tu as affaire, la très mauvaise nouvelle, c'est qu'il n'a peut-être pas été écrit avec un bon soucis de la gestion mémoire.
Bon courage.
Ce n'est plus nécessaire à utiliser les ivars. En plus, la méthode -saveTokenValue est totalement superflue.
En déclarant la propriété tokenValue avec l'attribut retain, c'est comme tu as aussi écrit la méthode d'accès setTokenValue qui contiendrait le code suivant
Mais c'est tous fait par le compilateur.
En plus, il faut ajouter la méthode -dealloc
Voilà exactement pourquoi je milite pour l'utilisation des @property et des setters/getters synthétisés automatiquement, et non pas par la gestion manuelle du code comme le suggère Mala : parce qu'aucun de ces 2 implémentations de setters n'est correcte, elles ont toutes les 2 des problèmes car manquent de certaines protections. Alors que le code du setter généré par LLVM lui est bien mieux pensé et optimisé et + secure. Le compilateur sait toujours mieux optimiser que vous et les @synthesize savent toujours mieux implémenter un setter/getter que vous, c'est pas la peine de se penser plus fort que le compilateur et les experts qui ont fait le code pour la synthétisation des propriétés
En particulier dans ce code il manque la protection pour le cas où tokenValue et _tokenValue pointent vers le même objet (EXC_BAD_ACCESS en vue dans ce cas), éventuellement un SpinLock dans le cas où on veut de l'atomicité, les annotations pour le compilateur sur les optimisations de branches sur les tests de if, etc...
Tout d'abord un grand merci à tous parce que vous m'avez appris vraiment plein de choses sur la gestion de la mémoire, et à titre perso j'y vois beaucoup plus clair qu'avant de poster sur ce forum !
Ensuite j'ai appliqué vos conseils et je n'ai plus d'avertissement mémoire et tout semble fonctionner plutôt bien !
Je vais vous schématiser ce que j'ai fait au final, me basant sur tous vos posts :
Dans la classe où se trouve la fameuse fonction du premier post, j'ai maintenant déclaré tokenValue de la sorte :
et je l'utilise comme cela :
Si j'ai bien compris c'est le setter de tokenValue qui s'occupera de faire le retain sur stringValue et de releaser au préalable tokenValue ?
Je pense que ça a été écrit comme ça à l'époque et vu que ça marchait, ils se sont arrêté là . Les données qui transitaient n'étaient pas importantes en quantité, donc ça ne crashait pas et maintenant c'est moi qui récolte tout ça :-D.
Encore une fois un grand merci à tous, Ali, Joanna Carter, FKDev, Mala, Alf et Céroce !
MoKeS
Rien ne t'empêche de faire :
De plus, le conseil donné plus haut par FKDEV de créer des "@autoreleasepool { ... }" à l'intérieur des boucles while et autres est très bon... mais il n'est pas non plus nécessaire d'en abuser.
En effet, cela est vraiment justifié quand la boucle risque d'être longue (plusieurs milliers d'itérations, genre) ou quand le corps de la boucle risque fortement de générer des quantités intermédiaire de mémoire assez conséquentes.
Par exemple pour du traitement d'image, allocation de grosses zones mémoire pour stocker des images intermédiaires de 800x800 pixels en RGBA soit ≈ 2,5Mo pour des images de cette taille) --> du coup ça vaut effectivement le coup de s'assurer, en encapsulant le corps de la boucle dans un "@autoreleasepool {}", que les objets autorelease sont vidés à chaque itération sans attendre trop longtemps.
Mais quand ce sont des boucles assez basiques et courtes (moins de 100 itérations) et à la quantité de mémoire créée à chaque itération qui reste faible (quelques NSString de sans doute moins de 1Ko chacune), comme ça a l'air d'être ton cas... c'est peut-être un peu de l'overkill pour de si petites zones de mémoire (qui seront de toute façon libérées, mais là avec le "@autoreleasepool {}" à l'intérieur de la boucle on les libère juste plus tôt à chaque itération de boucle plutôt que d'attendre la fin de la runloop).
Bon après ça fait pas de mal non plus, hein. C'est juste que c'est pas obligé d'en mettre à foison. Perso les seules fois où j'en ai mis c'est justement pour des boucles qui faisaient du traitement d'image et où chaque itération créait un pic d'allocation mémoire qu'il était bon de libérer rapidement sans attendre le dernier moment, sans quoi ça montait un peu trop et attendait un peu trop tard avant de libérer la place. Mais ça reste assez rare comme cas.
@AliGator
C'est ce que je me disais également au début, pour des NSString, pourquoi est-ce que je devrais m'inquiéter...
J'ai mis un petit compteur pour voir justement combien de fois on passait dans cette boucle while, et la fois qui faisait planter on passait 1700 fois dans cette boucle ! Et certains String en mémoire prenait presque 40 Ko d'après Instruments (avant que je mette le @property sur le tokenValue). Le reste du temps cela oscille entre 8 et 141 itérations.
Ce code fait partie d'un interpréteur de script, fait maison c'est donc assez aléatoire et dépend fortement de la taille du script à interpréter. Et ce bout de code justement, est encapsulé dans une autre boucle while...
@Joanna Carter
C'est vrai, je peux me passer de cette variable intermédiaire ! Merci !