On Thu, 31 Aug 2017 12:01:45 +0200 [email protected] said: > On Thu, Aug 31, 2017 at 06:44:52PM +0900, Carsten Haitzler wrote: > > On Thu, 31 Aug 2017 09:31:19 +0200 [email protected] said: > > > > > Good morning, > > > > > > On Thu, Aug 31, 2017 at 12:17:16AM -0700, Carsten Haitzler wrote: > > > > raster pushed a commit to branch master. > > > > > > > > http://git.enlightenment.org/core/efl.git/commit/?id=65d2dfc8925a183bf7f2709677d87edcd759e9c3 > > > > > > > > commit 65d2dfc8925a183bf7f2709677d87edcd759e9c3 > > > > Author: Carsten Haitzler (Rasterman) <[email protected]> > > > > Date: Thu Aug 31 16:12:57 2017 +0900 > > > > > > > > elm focus manager - dont leak child lists when updating focus order > > > > > > > > this should fix T5800 > > > > > > > > @fix > > > > --- > > > > src/lib/elementary/elm_box.c | 1 + > > > > src/lib/elementary/elm_grid.c | 1 + > > > > src/lib/elementary/elm_table.c | 1 + > > > > 3 files changed, 3 insertions(+) > > > > > > > > diff --git a/src/lib/elementary/elm_box.c b/src/lib/elementary/elm_box.c > > > > index b6a617dd2b..1c77ccd882 100644 > > > > --- a/src/lib/elementary/elm_box.c > > > > +++ b/src/lib/elementary/elm_box.c > > > > @@ -29,6 +29,7 @@ _focus_order_flush(Eo *obj, Elm_Box_Data *pd > > > > EINA_UNUSED) Eina_List *order = evas_object_box_children_get > > > > (wpd->resize_obj); > > > > efl_ui_focus_manager_calc_update_order(wpd->focus.manager, obj, > > > > order); > > > > + eina_list_free(order); > > > > > > Noep, you are passing the ownership of the list to the focus manager, > > > you should NOT use the list after that anymore, esp. not freeing it, > > > same for the other usages ... :) > > > > yeah... i figured that out right after. it fixed the leak... then led to > > more badness randomly in other cases. the issue is that failed method calls > > dont free allocated params (also it was missing an own() in the eo file too > > and other things). > > > > basically this will leak until eo can clean/free up the list passed in on a > > failed call (and eo file is fixed to say it owns and the code matches this > > too and frees the list inside the call all the time). > > > > so for now it duplicates until eo can/does do this. > > I know your manner of writing emails, so i thought twice about replying > to this. > > First of all lets break down what issues we had: > > - A missing own() in the .eo file, yep my misstake simple commit, simple fix > > - A missing free for the case a node was not known to the manager, which > is basically the insertion of { eina_list_free(order); return; } which > is not alot of things > > - eo is not freeing parameters that where defined with own(...) > > Number 1 & 2 are my misstakes and can be fixed very simple. > > Number 3 was caused by the sample code from T5800, which just creates > an evas (NOT and efl.ui.win) and then creates a elm.widget. On the one > hand if you just spend that second looking into your terminal you would > have seen that there is a error messages indicating that this box there > should REALLY not be there... or there should be a efl.ui.win. So we have > the first fuck up here that does not really need fixing in > efl.ui.focus.manager, but rather in your calling code. > > Now to your commit that "fixed" it. You have dropped out a optimization, > which was done to keep list duplication and clonging number low, without > even asking or trying to understand why. This was a lot of perf and > massif work to get those things down and small while working correctly. > Then you now went for the safe way and added if (manager) in every > single place where i called the order_update api. So #3 doesnt even > strike anymore, in the end those ifs where enough, without dropping out > the performance optimization. > > All in all this feels just like another "I need to fix this now without > asking why things where done that way". Could we somehow establish to at > least try to ask the person that wrote that code, and somehow coordinate > a fix that doesnt just hammer things in with a "deal with it" manner? > > Also on a slightly other note, just because code leaks memory or crashes > doesnt always indicate that we have a error on the called code. Thse > usecase where this elm.box was used is completly wrong and even tells > you that in a error messages. > > slightly salty greetings, > Marcel Hollerbach
none of this changes that fact that: 1. call the api on a null OR any invalid object and the list is leaked by current implementation of eo and design of the api 2. we shouldnt be leaking memory just because the win is not an efl ui win (ok - check if manager is null - i added that too to my fix), but this doesn't change #1 if it is called outside of these places where i added the checks 3. performance is important - but leaking is probably worse. any invalid use will leak until eo does the freeing. until then the api leaks by design. once it can free automatically on a failed call then go back to owning the input list. 4. the api already replies on allocing a new list first every time it is called in every instance. if performance really should be key then this should work without allocing a new list and all its members every time and probably ref some kind of accessor/iterator from the owner of the child list somehow this made a difference of 4gb used vs 20mb in the sample in the ticket. yes. the sample isn't great but it pointed out a hole. the hole of "if focus manager is not a valid one for any reason we end up with a leak and this is just an extreme example". if the list can be auto-freed on failed call by eo then as above... it could go back to taking ownership, but i think it'd be better to not even create a list to begin with. internally the code creates yet another node list too and allocs. so really this is probably only going to make a relatively small performance difference and not like "halve the speed". the alternative is to have leaks for these cases. eo isnt going to pick up these pieces soon as it'd take a lot more work. -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) [email protected] ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
