Re: ep_ memory is not garbage collected
On Mon, Aug 20, 2012 at 9:07 AM, Jakub Zawadzki
<darkjames-ws@...> wrote:
> On Sun, Aug 19, 2012 at 08:29:37AM -0400, Evan Huus wrote:
>> On Fri, Aug 17, 2012 at 10:02 AM, Jakub Zawadzki
>> <darkjames-ws@...> wrote:
>> > On Thu, Aug 16, 2012 at 08:54:54PM -0400, Evan Huus wrote:
>> >> On Wed, Aug 15, 2012 at 11:36 AM, Jakub Zawadzki
<darkjames-ws@...> wrote:
>> >> > From r43188 sequence of selecting new packet (cf_select_packet() in file.c) is:
>> >> > cf_read_frame()
>> >> > old_edt = cf->edt;
>> >> > cf->edt = epan_dissect_new() ; edt_refs++
>> >> > epan_dissect_run(cf->edt, ...)
>> >> > if (old_edt)
>> >> > epan_dissect_free(old_edt); ; edt_refs-- (edt_refs != 0, no gc)
>> >> >
>> >> > Old code was doing something like:
>> >> > cf_read_frame()
>> >> > epan_dissect_free(cf->edt); ; edt_refs-- (likely edt_refs == 0, gc ep memory)
>> >> > cf->edt = epan_dissect_new() ; edt_refs++
>> >> >
>> >> > I have working fix for this, but check below.
>> >>
>> >> Based on the log for r43188 I expect there's someway to force clear
>> >> the GtkTreeStore, so that old_edt can be unrefed (triggering a gc)
>> >> before cf->edt is re-allocated?
>> >>
>> >> Is your fix checked in or attached to a bug somewhere?
>> >
>> > Attached, but it's more workaround than fix.
>>
>> Looks sane enough as far as workarounds go. I don't know if there's
>> already a process for this, but I would commit that to trunk (with a
>> comment in the code pointing to this discussion).
>
> Well, I'd rather not commit it, cause it only fix part of the problem.
>
>> Once it's had a week or two of soak time, backport it to 1.8 and remove it from trunk.
>
> This is not required, r43188 is not part of 1.8
Whoops, that's right.
>> Any objections from the general list?
>>
>> I'll try and take a look at a proper fix for trunk at some point, but
>
> What do you think about reverting r42254 and using g_malloc() for data_source,
> like suggested by Anders? (https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5284#c26)
I believe that's the same idea Jeff considered and rejected earlier in
the comment thread for that bug [1]?
On Mon, Aug 20, 2012 at 11:03 AM, <mmann78@...> wrote:
> Would the patch from Bug 2047
> (https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2047) also be
> helpful/relevant?
It looks related, but it's not immediately obvious to me what it was
trying to accomplish. The description is... less than helpful. Anders,
do you remember what it was supposed to do?
---
I finally re-read a part of the comments on the bug (5284) and found
my original hash-table patch still sitting there. I don't know if it
still applies to trunk (emem doesn't change much, so I can hope), but
I think the general idea is still sound. If I remember correctly it's
basically a variant of the stack idea I floated earlier in this
thread. Anders wasn't comfortable with it (probably for good reason)
so he committed the ref-counting instead.
I've got a couple of interesting ideas to play with now for the next
time I get a chance.
Evan
[1] https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5284#c3
___________________________________________________________________________
Sent via: Wireshark-dev mailing list <wireshark-dev@...>
Archives: http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-request@...?subject=unsubscribe
RSS Feed