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 598896 - [GstRegistry] Cache lists of ElementFactory and TypeFindFactory
[GstRegistry] Cache lists of ElementFactory and TypeFindFactory
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 0.10.26
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 599147
 
 
Reported: 2009-10-19 07:21 UTC by Edward Hervey
Modified: 2009-10-24 08:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstregistry: Cache lists of elementfactory and typefindfactory. (4.65 KB, patch)
2009-10-19 07:22 UTC, Edward Hervey
rejected Details | Review
gstregistry: Add a cookie for detecting feature list changes (2.81 KB, patch)
2009-10-19 07:22 UTC, Edward Hervey
needs-work Details | Review
gstpluginfeature: API : new gst_plugin_feature_list_copy() method (3.26 KB, patch)
2009-10-21 07:51 UTC, Edward Hervey
needs-work Details | Review
registry: Cache element and typefind factories (4.57 KB, patch)
2009-10-21 07:52 UTC, Edward Hervey
needs-work Details | Review
gstregistry: Add a cookie for detecting feature list changes (2.96 KB, patch)
2009-10-24 08:12 UTC, Edward Hervey
committed Details | Review
gstpluginfeature: API : new gst_plugin_feature_list_copy() method (3.26 KB, patch)
2009-10-24 08:12 UTC, Edward Hervey
committed Details | Review
registry: Cache element and typefind factories (4.23 KB, patch)
2009-10-24 08:12 UTC, Edward Hervey
committed Details | Review

Description Edward Hervey 2009-10-19 07:21:15 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.
Comment 1 Edward Hervey 2009-10-19 07:22:01 UTC
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.
Comment 2 Edward Hervey 2009-10-19 07:22:49 UTC
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.
Comment 3 Edward Hervey 2009-10-19 07:23:15 UTC
The second patch obviously goes before the first one. I screwed up with git-bz :)
Comment 4 Edward Hervey 2009-10-21 07:51:44 UTC
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
Comment 5 Edward Hervey 2009-10-21 07:52:07 UTC
Created attachment 145924 [details] [review]
registry: Cache element and typefind factories

This avoids unneeded list/filtering if the registry hasn't changed
Comment 6 Edward Hervey 2009-10-21 07:53:57 UTC
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
Comment 7 Tim-Philipp Müller 2009-10-21 14:09:51 UTC
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)
Comment 8 Sebastian Dröge (slomo) 2009-10-22 08:48:10 UTC
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 ;)
Comment 9 Sebastian Dröge (slomo) 2009-10-22 08:53:16 UTC
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 ;)
Comment 10 Sebastian Dröge (slomo) 2009-10-22 08:58:27 UTC
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)
Comment 11 Edward Hervey 2009-10-22 09:08:59 UTC
(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.
Comment 12 Sebastian Dröge (slomo) 2009-10-22 09:25:36 UTC
(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
Comment 13 Edward Hervey 2009-10-24 08:12:41 UTC
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.
Comment 14 Edward Hervey 2009-10-24 08:12:45 UTC
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
Comment 15 Edward Hervey 2009-10-24 08:12:50 UTC
Created attachment 146155 [details] [review]
registry: Cache element and typefind factories

This avoids unneeded list/filtering if the registry hasn't changed
Comment 16 Sebastian Dröge (slomo) 2009-10-24 08:34:55 UTC
(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
Comment 17 Edward Hervey 2009-10-24 08:50:09 UTC
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.