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 621505 - Disable memory poisoning by default for releases
Disable memory poisoning by default for releases
Status: RESOLVED WONTFIX
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 0.10.30
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-06-14 07:48 UTC by Edward Hervey
Modified: 2010-06-23 09:00 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Edward Hervey 2010-06-14 07:48:56 UTC
Currently memory poisoning is always enabled. While this helps tracking down issue in development versions, it does enable some non-trivial computations which slow down caps operations.

gst_caps_append: Full caps_copy of the second argument
gst_caps_merge: Full caps copy of the second argumet
gst_caps_append_structure: Full copy of the structure to add

I'd push for not making it enabled by default for (pre-)releases.
Comment 1 Sebastian Dröge (slomo) 2010-06-14 07:54:37 UTC
Agreed, let's disable it for (pre-) releases.
Comment 2 Edward Hervey 2010-06-14 07:56:22 UTC
even worse than what I expected, it's also called by every API method calling those methods above (like gst_caps_intersect for ex :()
Comment 3 Benjamin Otte (Company) 2010-06-14 07:58:07 UTC
How many bugs do we catch due to poisoning anyway? Don't we usually catch those bugs with valgrind?
Comment 4 Tim-Philipp Müller 2010-06-14 08:40:01 UTC
> Currently memory poisoning is always enabled.

I'm not sure this is true in general. It's enabled in git because --enable-poisoning is one of the default args passed to configure in autogen.sh. It's not enabled if you unpack a tarball and run ./configure ...
Comment 5 Edward Hervey 2010-06-14 08:53:03 UTC
gah, good point. It's autogen.sh the culprit :(

As Benjamin noted though, is there still a point to have memory poisoning ?
Comment 6 Tim-Philipp Müller 2010-06-14 09:59:44 UTC
> As Benjamin noted though, is there still a point to have memory poisoning ?

Benjamin lives in a happy place where he's got full control of the stack and OS and valgrind works well and is generally fast enough. Other people may not be quite so lucky and may occasionally have to wrestle with puny embedded systems, proprietary operating systems etc.

I think it'd be nice to keep the option around to make debugging easier, even if we decide not to use it any more by default.
Comment 7 Edward Hervey 2010-06-14 14:23:37 UTC
Ok, so any objections for removing it from autogen.sh then but still keeping the option around for those who want/need it ?
Comment 8 David Schleef 2010-06-14 23:48:36 UTC
The overwriting of GstCaps/GstStructure structures caught several bugs when it was added.  It probably continues to have minor value by triggering valgrind warnings or segfaults earlier because of errors in external code, and the cost should be minor since the structure sizes are small and are likely in cache.  I'd like this to continue to be on, either via autogen.sh or GST_CVS=yes.  (Better would be a g_free variant that does poisoning based on an environment variable, but I digress.)

Copying the caps/structures at every operation seems rather expensive in comparison.  I'm probably missing something, but this seems to be geared more toward checking for bugs in core, not bugs in external code.
Comment 9 Tim-Philipp Müller 2010-06-22 18:00:40 UTC
I also think we should keep the current behaviour.

It's already disabled by default for releases, which is the most important thing I guess, so can we close this?
Comment 10 Edward Hervey 2010-06-23 09:00:17 UTC
Yah, let's close it. As long as people are aware of it when profiling on non-release checkouts I'm fine with it.