GNOME Bugzilla – Bug 775423
info: Add a 'flags' parameter to gst_debug_get_stack_trace
Last modified: 2016-12-12 18:13:42 UTC
See commit message
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 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.
(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.
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
Created attachment 341087 [details] [review] Minor fix to previous commit
Created attachment 341235 [details] [review] Minor fixes and add new symbols to documentation
Does that sound right to you now?
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? :)
(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'.
Ah right. Ok, let's just go with this then :)
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