Re: [Evolution-hackers] Possible memory leak
El lun, 14-01-2008 a las 14:33 +0100, Philip Van Hoof escribió: > I think this needs to be something like this: > > GSList *item = cc->status_stack; > cc->status_stack = g_slist_remove_link(cc->status_stack, item); > g_slist_free (item); Or just use g_slist_delete_link: cc->status_stack = g_slist_delete_link (cc->status_stack, cc->status_stack); AFAIK it frees the list node. -- Jose Dapena Paz <[EMAIL PROTECTED]> Igalia ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Possible memory leak
On Tue, 2008-01-15 at 11:55 +0100, Jose Dapena Paz wrote: > El lun, 14-01-2008 a las 14:33 +0100, Philip Van Hoof escribió: > > > I think this needs to be something like this: > > > > GSList *item = cc->status_stack; > > cc->status_stack = g_slist_remove_link(cc->status_stack, item); > > g_slist_free (item); > > Or just use g_slist_delete_link: > > cc->status_stack = g_slist_delete_link (cc->status_stack, cc->status_stack); > AFAIK it frees the list node. This is what got committed, indeed. -- Philip Van Hoof, freelance software developer home: me at pvanhoof dot be gnome: pvanhoof at gnome dot org http://pvanhoof.be/blog http://codeminded.be ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Possible memory leak
On Mon, 2008-01-14 at 14:33 +0100, Philip Van Hoof wrote: > I think I've found the leak ... > > g_slist_remove_link: > > Removes an element from a GSList, without freeing the element. The > removed element's next link is set to NULL, so that it becomes a > self-contained list with one element. > > Notice the "without freeing the element" > > Yet > > void > camel_operation_end (CamelOperation *cc) > { > ... > > g_free(s); > cc->status_stack = g_slist_remove_link(cc->status_stack, >cc->status_stack); > UNLOCK(); > > if (msg) { > cc->status(cc, msg, sofar, oftotal, cc->status_data); > g_free(msg); > } > } g_slist_delete_link() frees the GSList node, whereas g_slist_remove_link() just disconnects the node from the list. If I'm reading this right, the preceding logic frees the list node contents, so I think we just need to use g_slist_delete_link() in place of g_slist_remove_link(). One line change. void camel_operation_end (CamelOperation *cc) { ... s = cc->status_stack->data; ... g_free (s); cc->status_stack = g_slist_delete_link ( cc->status_stack, cc->status_stack); ... } ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Possible memory leak
I think I've found the leak ... g_slist_remove_link: Removes an element from a GSList, without freeing the element. The removed element's next link is set to NULL, so that it becomes a self-contained list with one element. Notice the "without freeing the element" Yet void camel_operation_end (CamelOperation *cc) { ... g_free(s); cc->status_stack = g_slist_remove_link(cc->status_stack, cc->status_stack); UNLOCK(); if (msg) { cc->status(cc, msg, sofar, oftotal, cc->status_data); g_free(msg); } } I think this needs to be something like this: GSList *item = cc->status_stack; cc->status_stack = g_slist_remove_link(cc->status_stack, item); g_slist_free (item); Can somebody with GSList know-how acknowledge this? On Mon, 2008-01-14 at 02:28 +0100, Philip Van Hoof wrote: > On Mon, 2008-01-14 at 01:47 +0100, Philip Van Hoof wrote: > > > I have this memory analysis tool that I'm willing to believe that tells > > me this line in camel-folder.c causes 381 times a leaked allocation of > > in total 5.95 kb. (I opened one folder of 1000 items). It seems to be > > the g_slice_alloc (which does a malloc since I disabled gslice) of the > > g_hash_node_new. > > I found it, this was my own mistake in camel-lite's adaptations. > > I have found another leak in camel-operation.c: > > The cc->status_stack = g_slist_prepend(cc->status_stack, s); in > camel_operation_start seems to be leaked each time it's called. > > I have 23 calls leaking 184 bytes. Various callers of > camel_operation_start (all of them, it seems) seem to create this leak. > > My version of camel-operation.c adds two fields (a total and a current > position in stead of a percentage, because my requirements where to give > accurate to-the-byte progress info, not rounded percentages). Those > changes are closely reviewed once more and don't seem to be causing this > leak. > > ps. I attached the differences that I have. I basically replaced "pc" > with a "sofar" and a "oftotal". > > > ___ > Evolution-hackers mailing list > Evolution-hackers@gnome.org > http://mail.gnome.org/mailman/listinfo/evolution-hackers -- Philip Van Hoof, freelance software developer home: me at pvanhoof dot be gnome: pvanhoof at gnome dot org http://pvanhoof.be/blog http://codeminded.be ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Possible memory leak
On Mon, 2008-01-14 at 01:47 +0100, Philip Van Hoof wrote: > I have this memory analysis tool that I'm willing to believe that tells > me this line in camel-folder.c causes 381 times a leaked allocation of > in total 5.95 kb. (I opened one folder of 1000 items). It seems to be > the g_slice_alloc (which does a malloc since I disabled gslice) of the > g_hash_node_new. I found it, this was my own mistake in camel-lite's adaptations. I have found another leak in camel-operation.c: The cc->status_stack = g_slist_prepend(cc->status_stack, s); in camel_operation_start seems to be leaked each time it's called. I have 23 calls leaking 184 bytes. Various callers of camel_operation_start (all of them, it seems) seem to create this leak. My version of camel-operation.c adds two fields (a total and a current position in stead of a percentage, because my requirements where to give accurate to-the-byte progress info, not rounded percentages). Those changes are closely reviewed once more and don't seem to be causing this leak. ps. I attached the differences that I have. I basically replaced "pc" with a "sofar" and a "oftotal". -- Philip Van Hoof, freelance software developer home: me at pvanhoof dot be gnome: pvanhoof at gnome dot org http://pvanhoof.be/blog http://codeminded.be --- camel-operation.c 2007-12-23 18:12:33.0 +0100 +++ /home/pvanhoof/repos/tinymail/trunk/libtinymail-camel/camel-lite/camel/camel-operation.c 2007-11-15 10:42:00.0 +0100 @@ -46,7 +46,7 @@ struct _status_stack { guint32 flags; char *msg; - int pc;/* last pc reported */ + int sofar, oftotal; unsigned int stamp; /* last stamp reported */ }; @@ -514,12 +514,14 @@ s = g_malloc0(sizeof(*s)); s->msg = msg; s->flags = 0; + s->sofar = 0; + s->oftotal = 100; cc->lastreport = s; cc->status_stack = g_slist_prepend(cc->status_stack, s); UNLOCK(); - cc->status(cc, msg, CAMEL_OPERATION_START, cc->status_data); + cc->status(cc, msg, CAMEL_OPERATION_START, 100, cc->status_data); d(printf("start '%s'\n", msg, pc)); } @@ -578,17 +580,17 @@ /** * camel_operation_progress: * @cc: Operation to report to. - * @pc: Percent complete, 0 to 100. + * @sofar: Amount complete + * @oftotal: Max amount (or, amount when completed) * * Report progress on the current operation. If @cc is NULL, then the - * currently registered operation is used. @pc reports the current - * percentage of completion, which should be in the range of 0 to 100. + * currently registered operation is used. @ * * If the total percentage is not know, then use * camel_operation_progress_count(). **/ void -camel_operation_progress (CamelOperation *cc, int pc) +camel_operation_progress (CamelOperation *cc, int sofar, int oftotal) { unsigned int now; struct _status_stack *s; @@ -608,7 +610,8 @@ } s = cc->status_stack->data; - s->pc = pc; + s->sofar = sofar; + s->oftotal = oftotal; /* Transient messages dont start updating till 4 seconds after they started, then they update every second */ @@ -632,7 +635,7 @@ UNLOCK(); if (cc) { - cc->status(cc, msg, pc, cc->status_data); + cc->status(cc, msg, sofar, oftotal, cc->status_data); g_free(msg); } } @@ -646,7 +649,7 @@ void camel_operation_progress_count (CamelOperation *cc, int sofar) { - camel_operation_progress(cc, sofar); + camel_operation_progress(cc, sofar, 100); } /** @@ -664,7 +667,8 @@ struct _status_stack *s, *p; unsigned int now; char *msg = NULL; - int pc = 0; + int sofar = 0; + int oftotal = 100; if (cc == NULL) cc = co_getcc(); @@ -693,13 +697,15 @@ if (p->flags & CAMEL_OPERATION_TRANSIENT) { if (p->stamp + CAMEL_OPERATION_TRANSIENT_DELAY < now) { msg = g_strdup(p->msg); - pc = p->pc; + sofar = p->sofar; + oftotal = p->oftotal; cc->lastreport = p; break; } } else { msg = g_strdup(p->msg); - pc = p->pc; + sofar = p->sofar; + oftotal = p->oftotal; cc->lastreport = p; break; } @@ -709,7 +715,8 @@ g_free(s->msg); } else { msg = s->msg; - pc = CAMEL_OPERATION_END; + sofar = CAMEL_OPERATION_END; + oftotal = 100; cc->lastreport = s; } g_free(s); @@ -718,7 +725,7 @@ UNLOCK(); if (msg) { - cc->status(cc, msg, pc, cc->status_data); + cc->status(cc, msg, sofar, oftotal, cc->status_data); g_free(msg); } } ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers