[Bf-committers] [Bf-blender-cvs] SVN commit: /data/svn/bf-blender [61066] trunk/blender/source/blender: code cleanup: warnings
Sergey Sharybin
sergey.vfx at gmail.com
Sun Nov 3 11:58:35 CET 2013
I run couple of times when i needed to modify IDs to figure out which exact
allocation wasn't freed. And i rather make it so code uses unique IDs from
the first place, not only when you actually run into memory leak. Not
regarding to bmesh or motrack, all the code is to follow the same rules.
It's really not a big deal to make strings a bit more unique. It's more
3sec to type when you work on the code first time, and it saves loads of
time when someone needs to find which exact allocation wasn't freed (making
IDs unique, re-compiling, not speaking of white hairs :)
So if there're no objections, i would just go ahead implementing proposal
above. Think this will solve both our needs :) Hopefully extra arg is not
gonna to affect on performance.
On Sun, Nov 3, 2013 at 4:46 PM, Campbell Barton <ideasman42 at gmail.com>wrote:
> This is probably more a matter of personal preference, you have a
> point, but I just never found this to be a problem - if it comes it it
> you could replace all occurances of `__func__` with `AT` and get line
> numbers for each alloc.
>
> The reason Id rather not use `AT` is it gives really long ID's (full
> path to source:line, and it doesnt group well in guarded alloc
> prints).
>
> A lot of the bmesh functions are really simple WRT allocations - and
> dont have a lot of complex branching with returns, just alloc some
> arrays, do some calculations, free and exit, the're really not hard to
> debug memory wise --- where most problems come from memory being used
> and freed by different functions. For these cases I'm all for having
> more descriptive ID's for each memory block.
>
> On Sun, Nov 3, 2013 at 9:44 PM, Sergey Sharybin <sergey.vfx at gmail.com>
> wrote:
> > Just got another idea. Use AT instead of __func__. This way we don't need
> > extra arguments to allocation functions and only need small wrapper for
> > them.
> >
> >
> > On Sun, Nov 3, 2013 at 4:14 PM, Sergey Sharybin <sergey.vfx at gmail.com
> >wrote:
> >
> >> That wouldn't be a problem if functions in blender didn't tend to be
> long
> >> with loads of branches and returns from the middle in some cases.
> >>
> >> I can see 6 allocations in bmo_wireframe_exec and all of them simply
> uses
> >> __func__. I have no idea how you might solve possible memory leak
> happening
> >> in such situation without going ahead and making IDs more unique.
> >> Especially if Campbell is not around and we urgently need to solve
> issues
> >> for artists ;)
> >>
> >> If someone forgets to update allocation string after move/copypaste we
> >> need to poke him.
> >>
> >> But what about this. Make it so __func__ is always appending as a prefix
> >> to string ID and you'll be passing var name (or even something more
> >> verbose) to allocation functions? Some wrapper function will make it
> >> totally transparent for all the cases.
> >>
> >> - Extend MemHead with one char* field. We could remove mmap easily and
> >> probably one of tags as well to prevent memory overhead.
> >> - Pass __func__ to allocation functions as a separate argument.
> >> - Add a macro wrapper which would look like: #define MEM_mallocN(size,
> >> str) MEM_mallocN_ex(size, __func__, str)
> >>
> >> Memleak prints will look like: "Unfreed block:
> >> bmo_wireframe_exec():verts_src"
> >>
> >> If someone could think of concatenation __func__ with str in a
> >> preprocessor, that would make changes even simpler (__func__ is a static
> >> char* so not sure it's possible).
> >>
> >>
> >> On Sun, Nov 3, 2013 at 3:41 PM, Campbell Barton <ideasman42 at gmail.com
> >wrote:
> >>
> >>> I've been using __func__ for simple cases:
> >>>
> >>> - where 1 or more arrays are allocated and freed within a single
> function.
> >>> (where the ID's wont show up in memory prints anyway - if they do
> >>> its a mem leak in the function).
> >>>
> >>> - where a single array is allocated and returned from a function
> >>> (where the function name is unique a useful reference).
> >>>
> >>> If there is some memory error - typically a direct link to the
> >>> function is a good start to investigating the problem, from there its
> >>> not so bad to edit an argument to differentiate (though in practice I
> >>> can't recall having to do this).
> >>>
> >>> One of the reasons I prefer this is in the past, I kept running into
> >>> string ID's that were based on vars or functions which were since
> >>> renamed (or code was moved elsewhere), and nobody thought to update
> >>> the names - so I'm happy with a reliable name even if its not always
> >>> unique.
> >>>
> >>> note - that we don't attempt unique string ID's in all other parts of
> >>> the code either.
> >>>
> >>> On Sun, Nov 3, 2013 at 8:25 PM, Sergey Sharybin <sergey.vfx at gmail.com>
> >>> wrote:
> >>> > Could see multiple allocations with the same block name of __func__.
> In
> >>> > this particular case it's rather harmless (for until someone goes
> >>> > initialize and adds return statement or so :), but i would rather be
> >>> strict
> >>> > about block names being unique named in general.
> >>> >
> >>> > Any good reason not using "bmo wireframe verts_{src,neg,pos}"
> instead of
> >>> > __func__?
> >>> >
> >>> >
> >>> > On Sun, Nov 3, 2013 at 11:19 AM, Campbell Barton <
> ideasman42 at gmail.com
> >>> >wrote:
> >>> >
> >>> >> Revision: 61066
> >>> >>
> >>> >>
> >>>
> http://projects.blender.org/scm/viewvc.php?view=rev&root=bf-blender&revision=61066
> >>> >> Author: campbellbarton
> >>> >> Date: 2013-11-03 05:19:55 +0000 (Sun, 03 Nov 2013)
> >>> >> Log Message:
> >>> >> -----------
> >>> >> code cleanup: warnings
> >>> >>
> >>> >> Modified Paths:
> >>> >> --------------
> >>> >> trunk/blender/source/blender/bmesh/operators/bmo_wireframe.c
> >>> >> trunk/blender/source/blender/editors/transform/transform.c
> >>> >>
> >>> >> Modified:
> trunk/blender/source/blender/bmesh/operators/bmo_wireframe.c
> >>> >> ===================================================================
> >>> >> --- trunk/blender/source/blender/bmesh/operators/bmo_wireframe.c
> >>> >> 2013-11-02 23:40:39 UTC (rev 61065)
> >>> >> +++ trunk/blender/source/blender/bmesh/operators/bmo_wireframe.c
> >>> >> 2013-11-03 05:19:55 UTC (rev 61066)
> >>> >> @@ -172,9 +172,9 @@
> >>> >> BMIter itersub;
> >>> >>
> >>> >> /* filled only with boundary verts */
> >>> >> - BMVert **verts_src = MEM_mallocN(sizeof(BMVert **) *
> >>> >> totvert_orig, __func__);
> >>> >> - BMVert **verts_neg = MEM_mallocN(sizeof(BMVert **) *
> >>> >> totvert_orig, __func__);
> >>> >> - BMVert **verts_pos = MEM_mallocN(sizeof(BMVert **) *
> >>> >> totvert_orig, __func__);
> >>> >> + BMVert **verts_src = MEM_mallocN(sizeof(BMVert *) *
> >>> >> totvert_orig, __func__);
> >>> >> + BMVert **verts_neg = MEM_mallocN(sizeof(BMVert *) *
> >>> >> totvert_orig, __func__);
> >>> >> + BMVert **verts_pos = MEM_mallocN(sizeof(BMVert *) *
> >>> >> totvert_orig, __func__);
> >>> >>
> >>> >> /* will over-alloc, but makes for easy lookups by index to
> keep
> >>> >> aligned */
> >>> >> BMVert **verts_boundary = use_boundary ?
> >>> >>
> >>> >> Modified: trunk/blender/source/blender/editors/transform/transform.c
> >>> >> ===================================================================
> >>> >> --- trunk/blender/source/blender/editors/transform/transform.c
> >>> 2013-11-02
> >>> >> 23:40:39 UTC (rev 61065)
> >>> >> +++ trunk/blender/source/blender/editors/transform/transform.c
> >>> 2013-11-03
> >>> >> 05:19:55 UTC (rev 61066)
> >>> >> @@ -4262,7 +4262,7 @@
> >>> >> }
> >>> >> }
> >>> >> BLI_snprintf(str + ofs, MAX_INFO_LEN - ofs, IFACE_(" or Alt)
> >>> Even
> >>> >> Thickness %s"),
> >>> >> - WM_bool_as_string(t->flag & T_ALT_TRANSFORM));
> >>> >> + WM_bool_as_string((t->flag & T_ALT_TRANSFORM)
> !=
> >>> 0));
> >>> >> /* done with header string */
> >>> >>
> >>> >>
> >>> >> @@ -6946,7 +6946,7 @@
> >>> >> }
> >>> >> }
> >>> >> ofs += BLI_snprintf(str + ofs, MAX_INFO_LEN - ofs, IFACE_("
> or
> >>> >> Alt) Expand to fit %s"),
> >>> >> - WM_bool_as_string(t->flag &
> >>> T_ALT_TRANSFORM));
> >>> >> + WM_bool_as_string((t->flag &
> >>> T_ALT_TRANSFORM)
> >>> >> != 0));
> >>> >> }
> >>> >>
> >>> >> static void applySeqSlideValue(TransInfo *t, const float val[2])
> >>> >>
> >>> >> _______________________________________________
> >>> >> Bf-blender-cvs mailing list
> >>> >> Bf-blender-cvs at blender.org
> >>> >> http://lists.blender.org/mailman/listinfo/bf-blender-cvs
> >>> >>
> >>> >
> >>> >
> >>> >
> >>> > --
> >>> > With best regards, Sergey Sharybin
> >>> > _______________________________________________
> >>> > Bf-committers mailing list
> >>> > Bf-committers at blender.org
> >>> > http://lists.blender.org/mailman/listinfo/bf-committers
> >>>
> >>>
> >>>
> >>> --
> >>> - Campbell
> >>> _______________________________________________
> >>> Bf-committers mailing list
> >>> Bf-committers at blender.org
> >>> http://lists.blender.org/mailman/listinfo/bf-committers
> >>>
> >>
> >>
> >>
> >> --
> >> With best regards, Sergey Sharybin
> >>
> >
> >
> >
> > --
> > With best regards, Sergey Sharybin
> > _______________________________________________
> > Bf-committers mailing list
> > Bf-committers at blender.org
> > http://lists.blender.org/mailman/listinfo/bf-committers
>
>
>
> --
> - Campbell
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers
>
--
With best regards, Sergey Sharybin
More information about the Bf-committers
mailing list