GNOME Bugzilla – Bug 598896
[GstRegistry] Cache lists of ElementFactory and TypeFindFactory
Last modified: 2009-10-24 08:50:43 UTC
searching for the list of all available elementfactory and typefindfactory is a common task in GStreamer. But yet it always re-scans the full list of available pluginfeatures even if that list hasn't changed.
Created attachment 145765 [details] [review] gstregistry: Cache lists of elementfactory and typefindfactory. These two searches are the most common ones done on the registry. We use the feature list cookie to invalidate the list if needed.
Created attachment 145766 [details] [review] gstregistry: Add a cookie for detecting feature list changes We also create a private structure, since we will need to add more data there in following patches.
The second patch obviously goes before the first one. I screwed up with git-bz :)
Created attachment 145923 [details] [review] gstpluginfeature: API : new gst_plugin_feature_list_copy() method This allows copying AND incrementing the refcount at the same time, avoiding a double iteratio of the GList
Created attachment 145924 [details] [review] registry: Cache element and typefind factories This avoids unneeded list/filtering if the registry hasn't changed
There was a bug in my previous patch, whereby we wouldn't increment the initial cached copy. I also added a new API for doing a (slightly) faster copy-and-ref of plugin feature lists
Only glanced at these quickly, but still three random comments: - it's a bit nasty to rely on internal GLib implementation details for GList (might be ok for now, and not a problem in practice, but still not very nice) - would be nicer to use GObject's private instance structure stuff instead of foo->priv = g_new0 (FooPrivate) - maybe this microoptimisation of the GList copying + ref'ing shows that the approach taken is wrong and a different approach should be taken by the users of this API? (different from: get list of all xyz and then iterate and filter)
Review of attachment 145766 [details] [review]: What Tim said, use the GObject instance private stuff here. Not that it matters much (because there's usually only one GstRegistry instance) but it's cleaner ;)
Review of attachment 145923 [details] [review]: - Use g_list_alloc() instead of GSlice, you never know if GLib will change allocations of list nodes again in the future - You could use g_list_append() instead of the manual twiddling with the next and prev pointers... unless the function call is too expensive for you :) g_list_append() runs in O(1) if you append a new node to the last element btw but you know that ;)
Review of attachment 145924 [details] [review]: Looks good to me but to reduce code duplication better make the get-or-create function a bit more generic: GList * gst_registry_get_element_factory_list (GstRegistry *registry, GList **list, guint32 *cookie, GType type)
(In reply to comment #8) > Review of attachment 145766 [details] [review]: > > What Tim said, use the GObject instance private stuff here. Not that it matters > much (because there's usually only one GstRegistry instance) but it's cleaner > ;) That private data code in gobject/gtype.c is ugly/slow beyond belief. Until that's fixed, I have no intention of adding that in optimisations. I could definitely have a look at speeding up that gtype code though. (In reply to comment #9) > Review of attachment 145923 [details] [review]: > > - Use g_list_alloc() instead of GSlice, you never know if GLib will change > allocations of list nodes again in the future > - You could use g_list_append() instead of the manual twiddling with the next > and prev pointers... unless the function call is too expensive for you :) > g_list_append() runs in O(1) if you append a new node to the last element btw > but you know that ;) I'm just wondering if it's worth it. Having to call two external functions (which do checks) over a list.. is basically gonna kill the optimisation. I'm either completely removing it or keeping as is. (In reply to comment #10) > Review of attachment 145924 [details] [review]: > > Looks good to me but to reduce code duplication better make the get-or-create > function a bit more generic: > GList * gst_registry_get_element_factory_list (GstRegistry *registry, GList > **list, guint32 *cookie, GType type) Yah, I thought about that... I'll try to write generic new API functions for requesting lists with one already present. Thanks for the review.
(In reply to comment #11) > (In reply to comment #8) > > Review of attachment 145766 [details] [review] [details]: > > > > What Tim said, use the GObject instance private stuff here. Not that it matters > > much (because there's usually only one GstRegistry instance) but it's cleaner > > ;) > > That private data code in gobject/gtype.c is ugly/slow beyond belief. Until > that's fixed, I have no intention of adding that in optimisations. > I could definitely have a look at speeding up that gtype code though. It's not going to make anything slower here because you get it once in your instance_init as you do it now. Getting it always when you need it is far too expensive for everything. But you agreed to that on IRC already ;) > (In reply to comment #9) > > Review of attachment 145923 [details] [review] [details]: > > > > - Use g_list_alloc() instead of GSlice, you never know if GLib will change > > allocations of list nodes again in the future > > - You could use g_list_append() instead of the manual twiddling with the next > > and prev pointers... unless the function call is too expensive for you :) > > g_list_append() runs in O(1) if you append a new node to the last element btw > > but you know that ;) > > I'm just wondering if it's worth it. Having to call two external functions > (which do checks) over a list.. is basically gonna kill the optimisation. > I'm either completely removing it or keeping as is. g_list_alloc() does no checks, it's just a wrapper around g_slice_new0() but if you use it, nothing will break when GLib switches to GSlice2 or GSuperAllocator :) Argument for g_list_append() is fine though, just keep that code as is and only call g_list_alloc(). The prev/next/data design of GList can't change anyway without breaking ABI/API
Created attachment 146153 [details] [review] gstregistry: Add a cookie for detecting feature list changes We also create a private structure, since we will need to add more data there in following patches.
Created attachment 146154 [details] [review] gstpluginfeature: API : new gst_plugin_feature_list_copy() method This allows copying AND incrementing the refcount at the same time, avoiding a double iteratio of the GList
Created attachment 146155 [details] [review] registry: Cache element and typefind factories This avoids unneeded list/filtering if the registry hasn't changed
(In reply to comment #15) > Created an attachment (id=146155) [details] [review] > registry: Cache element and typefind factories > > This avoids unneeded list/filtering if the registry hasn't changed The get_{typefind,element}_factory_list() functions need locking for the copying as discussed on IRC
Committed with locking fixed commit c79ed99bab51a769924bfb33abb6b921df368829 Author: Edward Hervey <bilboed@bilboed.com> Date: Wed Oct 21 09:45:47 2009 +0200 registry: Cache element and typefind factories. Fixes 598896 This avoids unneeded list/filtering if the registry hasn't changed commit ebee2588062645afa75f5a800350bb5b14e53ecf Author: Edward Hervey <bilboed@bilboed.com> Date: Wed Oct 21 09:40:49 2009 +0200 gstpluginfeature: API : new gst_plugin_feature_list_copy() method This allows copying AND incrementing the refcount at the same time, avoiding a double iteratio of the GList commit 9e792ee5b8aa7f5e4f3addea3ba579b84581a4a0 Author: Edward Hervey <bilboed@bilboed.com> Date: Sat Oct 24 10:05:59 2009 +0200 gstregistry: Add a cookie for detecting feature list changes We also create a private structure, since we will need to add more data there in following patches.