GNOME Bugzilla – Bug 612540
Build releases (X.Y.0) with -DG_DISABLE_CAST_CHECKS
Last modified: 2010-04-09 09:18:27 UTC
Currently all our .0 releases are being built with the expensive cast type checking enabled. While this helps for debugging purposes (git versions), it does introduce an overhead which is useless for releases. Disabling those type checks will not change in any way the behaviour of GStreamer and the various plugins (the only difference being that it will not output a warning message on the console, but if something crashed/errored before it will still behave the same). FWIW, gtk+ does the same for their stable releases.
+1 from me.
Seem ok to me. I wouldn't want to miss out on useful debugging information from and debugging capabilities (via G_DEBUG=fatal_warnings) of distro users filing bugs, but in my experience almost all of that comes from g_return_if_fail (GST_IS_FOO (bar))); rather than the runtime casts checks, so I'm ok with it.
Ok, wanted to get a general feel, but now it's clear that nobody's against it. I'll cook up some patches.
Any news on this? Might be nice to get in before next core/base/good freeze :)
I wonder if it's useful to stuff this into some common macro instead of touching configure.ac everywhere. But I didn't find one. Any suggestions?
AG_GST_GLIB_CHECK() into GLIB_CFLAGS maybe.
Created attachment 157014 [details] [review] Add -DG_DISABLE_CAST_CHECKS for (pre)releases
I think this may make it harder for 3rd party developers, so I'm not in favor of putting it in releases. This should really be done by the distributors, not in the source package. Ie, maybe it should be a --enable configure flag instead.
Could you elaborate on what requirements you have? I don't understand what's so different between a source release, a distro release and a third party release.
Source releases are used by developers. Distro releases (ie rpms or debs) are for end-users. My requirement is that its easier to spot bugs in my application if the typechecks are there. I dont think we have g_return_if_fail(!G_IS_TYPE_XX()) everywhere.
Olivier: I agree, but I don't think the proposed change would have a noticable impact on debuggability in practice. Keep in mind that this only affects casts within GStreamer core/libs and within GStreamer plugins, not external plugins such as farsight or applications. Where you would have gotten a warning and a crash, you now only get a crash without the cast warning. This is not the kind of thing that's hard to debug. Besides, these kind of failures hardly ever happen at all. Can you remember the last time you ran into this kind of thing (triggered by GStreamer code)? I can't. And I do think we have lots of guards in all important places. If there are guards missing somewhere, we should add some. The only thing I'm unsure about is whether this flag should be enabled/disabled based on whether git is used or not. I think we should probably either always use it or never use it.
> Besides, these kind of failures hardly ever happen at all. Can you remember the > last time you ran into this kind of thing (triggered by GStreamer code)? I > can't. Just to clarify: I'm not saying that I'm rarely seeing this kind of issue, it's just that we have guards almost everywhere, so that these kind of issues are usually caught by function guards rather than cast macros, at least in my experience.
I think binary distributors possibly want to disable the cast macros (like we do on Maemo). But that the source version shouldn't. If you're building from source, you're probably a developer and beefy hardware so it probably doesn't matter.
Should we therefore add a configure option to *NOT* add that flag for releases ? GTK+ does this, and I haven't seen *anybody* complain about that (and for running gtk unstable, trust me... there's a *LOT* more warning spurting than with GStreamer). I'd prefer the default to be to have those cast checks disabled by default for releases (let's face it most binary distros don't have time to care about all the options), but leave an option for THOSE WHO KNOW to leave those checks in. Also : if you're that implicated in the development and it matters that much... you should be testing your code against git/pre-releases which would leave those checks in.
I've always been thinking that the source tarball by default is configured how a distributor should use it. This way upstream can balance the need for debugging information with the need for performance as they see fit. This frees packagers from the burden of having to know about all this stuff. Developers using GStreamer itself will either run git checkout (and then not get this flag) or they will run packages. And then they fall into the same category as the package groups in that we need to decide what's best for them. Which is what we do here. I might be convinced to add an option for this if anyone was able to tell me (without looking it up) how to disable this flag in gtk - which has been using it for years and nobody ever complained afaik.
Review of attachment 157014 [details] [review]: Looks good to me. Just wondering whether we'd want only the releases (nano == 0) to have that. The reasoning would be that people testing the pre-releases would see the warnings...
I decided against that because I thought prereleases should be as close as possible to real releases.
(In reply to comment #10) > Source releases are used by developers. > Distro releases (ie rpms or debs) are for end-users. > > My requirement is that its easier to spot bugs in my application if the > typechecks are there. I dont think we have g_return_if_fail(!G_IS_TYPE_XX()) > everywhere. This is about G_DISABLE_CAST_CHECKS, that only disables checking in a=G_TYPE_XX(b) If we disable this we have an ordinary cast. g_return_if_fail() is disabled by G_DISABLE_CHECKS (which is not so safe thing to do) and G_IS_TYPE_XX() can obviously not be disabled at all.
Ok, so can we please get this in now in one way or another? :) To make everybody happy I'd vote for a --enable-gobject-cast-checks=[yes,auto,no] where yes and no should be obvious and auto is yes for GIT and no for pre-releases and releases. Any objections? If not I'll create such a patch later today if nobody else is faster
Created attachment 158259 [details] [review] gst-glib2.m4: Add configure parameter to disable GObject type checks ...and disable them by default for pre-releases and releases. Fixes bug #612540.
Looks good to me ! Commit to common and update in core/base/good
I still think we shouldn't enable/disable this based on git/(pre-)releases, but just enable it all the time, unless disabled explicitly via configure.
> just enable it all the time, unless disabled explicitly via configure. And with 'enable' I meant 'disable the type checks in the cast macros' of course.
(In reply to comment #22) > I still think we shouldn't enable/disable this based on git/(pre-)releases, but > just enable it all the time, unless disabled explicitly via configure. I prefer to have cast checks on when developing new code. It's rare that I mess it up, but it happens that I mess up variables in initializers and it helps when it's caught on the first try. I agree that it's rather useless for bugfixing and testing work though.
commit fc8586793dc45a343772c1e25bf8dd45bbb64210 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Apr 9 08:17:45 2010 +0200 gst-glib2.m4: Add configure parameter to disable GObject type checks ...and disable them by default for pre-releases and releases. Fixes bug #612540.