After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 775423 - info: Add a 'flags' parameter to gst_debug_get_stack_trace
info: Add a 'flags' parameter to gst_debug_get_stack_trace
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-30 18:47 UTC by Thibault Saunier
Modified: 2016-12-12 18:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
info: Add a 'full' parametter to gst_debug_get_stack_trace (4.23 KB, patch)
2016-11-30 18:47 UTC, Thibault Saunier
reviewed Details | Review
info: Add a 'flags' parametter to gst_debug_get_stack_trace (4.99 KB, patch)
2016-11-30 19:49 UTC, Thibault Saunier
none Details | Review
Minor fix to previous commit (4.99 KB, patch)
2016-11-30 20:03 UTC, Thibault Saunier
none Details | Review
Minor fixes and add new symbols to documentation (5.46 KB, patch)
2016-12-02 11:59 UTC, Thibault Saunier
committed Details | Review

Description Thibault Saunier 2016-11-30 18:47:48 UTC
See commit message
Comment 1 Thibault Saunier 2016-11-30 18:47:52 UTC
Created attachment 341081 [details] [review]
info: Add a 'full' parametter to gst_debug_get_stack_trace

This is an API break but that API has not been released yet.

Retrieving source file and line numbers is pretty
expensive while getting a stack trace, this new argument
allows the user to decide to retrieve a backtrace
without those infos instead which is much faster.

For example running $ GST_LEAKS_TRACER_STACK_TRACE=1 GST_DEBUG=GST_TRACER:7 \
GST_TRACERS=leaks time gst-launch-1.0 videotestsrc num-buffers=1 ! fakesink:

* With simple stack traces:

    0.04s user 0.02s system 99% cpu 0.060 total

* With full stack traces:

    0.66s user 0.23s system 96% cpu 0.926 total
Comment 2 Tim-Philipp Müller 2016-11-30 19:05:00 UTC
Comment on attachment 341081 [details] [review]
info: Add a 'full' parametter to gst_debug_get_stack_trace

Funny, when I saw the original addition I thought to myself 'maybe we should add a flags argument for future use' ;)

Maybe we should make this a flags arg or do we reckon that a simple bool will definitely be enough?

I also wonder if it makes sense to add the option via another func for a 'blob of data' (e.g. array of pointers or whatever) to be retrieved as stack trace, and then another function to turn that into symbols/files/line numbers. The leak tracer for example should only really need to resolve the details when it prints the leak, not during normal operation.
Comment 3 Thibault Saunier 2016-11-30 19:48:43 UTC
(In reply to Tim-Philipp Müller from comment #2)
> Comment on attachment 341081 [details] [review] [review]
> info: Add a 'full' parametter to gst_debug_get_stack_trace
> 
> Funny, when I saw the original addition I thought to myself 'maybe we should
> add a flags argument for future use' ;)
> 
> Maybe we should make this a flags arg or do we reckon that a simple bool
> will definitely be enough?

DONE, it is indeed a good idea, I think in the future we might want to also allow retrieving back trace for all threads.

> I also wonder if it makes sense to add the option via another func for a
> 'blob of data' (e.g. array of pointers or whatever) to be retrieved as stack
> trace, and then another function to turn that into symbols/files/line
> numbers. The leak tracer for example should only really need to resolve the
> details when it prints the leak, not during normal operation.

I had also tried that but it depends a lot on what tool we use, with backtrace for example it makes no sense. I tried with libunwind/libdw and keeping the information around through a dwlf session means to keep a FD open per stack trace and I was exceeding the limit of opened fds almost right away running the leak tracer. We would need to retrieve the info, and put it in dedicated structures (like struct {gint line_number, gchar *symbol_name, ...}) but I do not see a good reason to do that as we would not avoid retrieving the info 'at runtime' which is the expensive operation.
Comment 4 Thibault Saunier 2016-11-30 19:49:00 UTC
Created attachment 341086 [details] [review]
info: Add a 'flags' parametter to gst_debug_get_stack_trace

This is an API break but that API has not been released yet.

We are passing a flag rather than a simple boolean as we can imagine
to implement more features in the future for example to retrieve a
stack trace for all the threads, etc..

Retrieving source file and line numbers is pretty
expensive while getting a stack trace, this new argument
allows the user to decide to retrieve a backtrace
without those infos instead which is much faster.

For example running $ GST_LEAKS_TRACER_STACK_TRACE=1 GST_DEBUG=GST_TRACER:7 \
GST_TRACERS=leaks time gst-launch-1.0 videotestsrc num-buffers=1 ! fakesink:

* With simple stack traces:

    0.04s user 0.02s system 99% cpu 0.060 total

* With full stack traces:

    0.66s user 0.23s system 96% cpu 0.926 total
Comment 5 Thibault Saunier 2016-11-30 20:03:09 UTC
Created attachment 341087 [details] [review]
Minor fix to previous commit
Comment 6 Thibault Saunier 2016-12-02 11:59:00 UTC
Created attachment 341235 [details] [review]
Minor fixes and add new symbols to documentation
Comment 7 Thibault Saunier 2016-12-12 14:25:24 UTC
Does that sound right to you now?
Comment 8 Tim-Philipp Müller 2016-12-12 14:46:40 UTC
Looks fine to me, just wondering about two things:

1) should we make it FLAG_FULL or FLAG_MINIMAL ? (ie. invert the meaning)

2) as far as I can tell all callers use FULL now - is that on purpose, or did I miss a call? :)
Comment 9 Thibault Saunier 2016-12-12 14:56:13 UTC
(In reply to Tim-Philipp Müller from comment #8)
> Looks fine to me, just wondering about two things:
> 
> 1) should we make it FLAG_FULL or FLAG_MINIMAL ? (ie. invert the meaning)

I am not sure why we would do that though :)

> 2) as far as I can tell all callers use FULL now - is that on purpose, or
> did I miss a call? :)

Right, because this is just paving the way toward https://bugzilla.gnome.org/show_bug.cgi?id=775541 where the user of the leak tracer get the possibility to choose it as an option, and by default it becomes 'simple'.
Comment 10 Tim-Philipp Müller 2016-12-12 15:02:17 UTC
Ah right. Ok, let's just go with this then :)
Comment 11 Thibault Saunier 2016-12-12 18:13:42 UTC
commit 33616d47becf743c038d10819e013bf0d0314804
Author: Thibault Saunier <tsaunier@gnome.org>
Date:   Wed Nov 30 15:10:48 2016 -0300

    info: Add a 'flags' parametter to gst_debug_get_stack_trace
    
    This is an API break but that API has not been released yet.
    
    We are passing a flag rather than a simple boolean as we can imagine
    to implement more features in the future for example to retrieve a
    stack trace for all the threads, etc..
    
    Retrieving source file and line numbers is pretty
    expensive while getting a stack trace, this new argument
    allows the user to decide to retrieve a backtrace
    without those infos instead which is much faster.
    
    For example running $ GST_LEAKS_TRACER_STACK_TRACE=1 GST_DEBUG=GST_TRACER:7 \
    GST_TRACERS=leaks time gst-launch-1.0 videotestsrc num-buffers=1 ! fakesink:
    
    * With simple stack traces:
    
        0.04s user 0.02s system 99% cpu 0.060 total
    
    * With full stack traces:
    
        0.66s user 0.23s system 96% cpu 0.926 total
    
    https://bugzilla.gnome.org/show_bug.cgi?id=775423