Page MenuHomePhabricator

Freeing Global Memory list on destructor
ClosedPublic

Authored by ali.alzyod on Apr 8 2019, 9:50 AM.

Details

Summary

Freeing Global Memory list on destructor.

Issue with global list item, used to same styles. (in destructor we do not remove styles from it, which will cause memory leak)

Test Plan
#define EFL_EO_API_SUPPORT 1
#define EFL_BETA_API_SUPPORT 1
#include<Eina.h>
#include<Efl.h>
#include <Elementary.h>

EAPI_MAIN int
elm_main(int argc, char **argv)
{
   Evas_Object *win,*textblock;

   elm_policy_set(ELM_POLICY_QUIT, ELM_POLICY_QUIT_LAST_WINDOW_CLOSED);

   win = elm_win_util_standard_add("Main", "App");
   elm_win_autodel_set(win, EINA_TRUE);
  
  char * style = "DEFAULT='font=Sans font_size=30 style=shadow color=#F00 text_class=entry align=center shadow_color=#00F color=#000'";

  // Global internal list will add 1000 style to it
  // and it should not do that because styles is not referenced by any object
  for(int i = 0 ; i < 1000; i++){
   textblock = efl_add(EFL_CANVAS_TEXT_CLASS,win);
   efl_canvas_text_style_set(textblock,"DEFAULT",style);
   efl_del(textblock);
  }
   evas_object_size_hint_weight_set(textblock,EVAS_HINT_EXPAND,EVAS_HINT_EXPAND);
   evas_object_size_hint_align_set(textblock,EVAS_HINT_FILL,EVAS_HINT_FILL);
   evas_object_show(textblock);
   
 
   evas_object_move(textblock,0,0);
   evas_object_resize(textblock,320,480);
   evas_object_resize(win,320,480);
   
   evas_object_show(win);
   elm_run();

   return 0;
}
ELM_MAIN()

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ali.alzyod created this revision.Apr 8 2019, 9:50 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

ali.alzyod requested review of this revision.Apr 8 2019, 9:50 AM
ali.alzyod retitled this revision from Speed up setting text (check if same text is already set), fix setting same text pointer to Freeing Global Memory list on destructor.Apr 8 2019, 9:52 AM
ali.alzyod edited the summary of this revision. (Show Details)
ali.alzyod added reviewers: woohyun, bowonryu.
segfaultxavi requested changes to this revision.Apr 9 2019, 1:31 AM
segfaultxavi added a subscriber: segfaultxavi.

Be careful, you merged two different things here!

This revision now requires changes to proceed.Apr 9 2019, 1:31 AM
ali.alzyod updated this revision to Diff 21292.Apr 9 2019, 8:17 AM

remove merged changes

ali.alzyod edited the test plan for this revision. (Show Details)Apr 9 2019, 8:22 AM
segfaultxavi resigned from this revision.Apr 9 2019, 9:06 AM

Thanks. Will let an expert review the code, though :)

99% of the time, it is a bad idea to use global variables inside library

bu5hm4n accepted this revision.Apr 17 2019, 11:32 PM
bu5hm4n added a subscriber: bu5hm4n.

It is not a bad idea to use a global variable, this is a cache, it must be global to be useful. If this variable would not exist, then every textblock would again and again create its own textblock_styles, again and again. That is nothing that we should do.
However, this here fixes a invalid pointer in the list.

This revision is now accepted and ready to land.Apr 17 2019, 11:32 PM
Closed by commit rEFL80ffed5d8563: Freeing Global Memory list on destructor (authored by ali.alzyod, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyApr 17 2019, 11:33 PM
This revision was automatically updated to reflect the committed changes.

@bu5hm4n It is bad Idea because it lead to :
1- Hard to follow memory leaks,
2- With any updating to the code,There is high probability to accidentally forget about global vars ,(This case shows my point) (like this memory leak)

To clear my point : Caching is good Idea, but using global variables is not good Idea most of the time

ali.alzyod edited the test plan for this revision. (Show Details)Apr 18 2019, 12:17 AM

This case does not prove you point, it does not matter if this list is in the object or global. Its was forgotten, no matter where the variable is located. You will likely find a lot of such cases, but undependend from if it is a global var or not.
And the same applies to memleaks, they are hard to follow no matter if they are global or not. valgrind etc. are usefull to find them.

hmmm,

Normally inside destructor you will check all data members for your object. (so you will not forget them), but for global maybe you did not know that you have global ones.

Anyway memory leak gone :)

raster added a subscriber: raster.Apr 18 2019, 1:37 AM

@bu5hm4n is right - this has nothing to do with globals. it'd be the same as if obj->styles = eina_list_remove(obj->styles, ts); was missing in the free func for the obj.

specifically this has to do with de-duplication so if you have 1000 textblock objects you don't create 1000 styles if they all share the same one and TRACKING all of those in a library-private global. we have tonnes of such globals in efl because it's the nicest way to do things. evas has image and font caches hidden behind the scenes that are shared across many canvases etc. - edje does too. the alternative is the api exposing this with some kind of "library context global handle" and then every call has to pass this handle around - it just makes for more work by the user of the api (having to create and store this handle in their own global 99.9999% of the time anyway, and also having to pass it in again and again etc.). globals are not always the best solution but in many cases the sanest and simplest. they don't lead to more leaks - they just change where the leak can be found.

@raster again I am not against caching, caching is great.

Maybe because of our style in programming we tried to avoid Global variables as much as possible, sometimes when we dig hard in global variables we may find some scenario for leak. (more probable than others)

So for example here instead of accessing global variables directly, we can make nicer way to handle them.
for example here : instead of accessing "_style_cache" global var. we may create:
GetSharedStyle();
RemoveSharedStyle();

but those functions then access the globals anyway so it's just pushing the problem away one level into another function, and if someone forgets a RemoveSharedStyle() call just like the list remove above - same thing happens. it's not really any different. these functions are indeed good to avoid copy & paste code if you have lots of places that do the same list remove and to be fair - efl has a lot of things like this that have crept in over many years.this is an area that efl could do with some attention to look for "copy & paste" code even if it looks different on the surface and possibly has minor difference (that could be bugs - careful thought and attention would need to be applied to see if they are really intended or not). if these bits of code can be refactored to share more common functions like you describe above where there is duplication, code size can be reduced and the chances of future problems where a list remove is forgotten in 1 case but not the other reduce. so to some extent you have a point, but in this case there isn't anything really to gain. indirection to a function which is all of 1 or 2 lines of code is generally not that useful. if its a larger blob of complexity then it'd probably worth a look. :) i hope this serves as some encouragement and guidance. in this case you found a bug, but your solution is IMHO with the above functions won't help IMHO, but there are enough other cases where this is really needed and you'll get little argument over :) i know the code exists. i know i've been guilty of doing it myself often enough. :)