GNOME Bugzilla – Bug 756738
gst_registry_plugin_filter() and gst_registry_feature_filter() callbacks can't use any API using the registry
Last modified: 2016-03-26 11:22:30 UTC
See https://bugzilla.mozilla.org/show_bug.cgi?id=1215579 Problem is that both functions are taking the registry lock for the whole time, and any other registry API would do that too, leading to a deadlock. You might want to e.g. call gst_plugin_feature_check_version() in the callback, which is currently not possible. We should document this and probably provide functions around GstIterator too.
Created attachment 313514 [details] [review] registry: Add iterators for the registry features and plugins The feature and plugin filters are taking the registry lock for the whole time, making it impossible to call any other GstRegistry API from their callback. With the iterators this is possible.
Created attachment 313515 [details] [review] registry: Add warning to the filter functions about not calling GstRegistry API It will deadlock.
I don't know, this doesn't really strike me as something that usually requires thread-safe iteration. Who wants to iterate the registry using GstIterator? We could just retrieve a full GList of plugins/features (with ref) in these functions at the start (with lock held), and then iterate over that retrieved list instead of the original list in the registry. I think that's what the user would expect, and the extra overhead seems negligible.
Created attachment 313546 [details] [review] registry: allow plugin and feature filter funcs to call registry API [alternative patch]
Review of attachment 313546 [details] [review]: If you think the copying is ok, go ahead :) That's exactly what I wanted to prevent with the iterator
Right, there is more overhead. Not sure if these functions are ever performance critical, I'm tempted to say not, but of course it's always annoying to add more overhead. Another possibility would be to make the existing filter functions use an iterator internally, but this would mean keeping track of items checked, which may well be more overhead than the list copy/free in many cases.
Using an iterator internally and remembering which items we saw already seems like more overhead to me :) If GList was refcounted (we could make it so), we could make the common operation (read-only usage) cheap and the uncommon usage (adding/removing things) expensive. So the other way around than in your patch (and also the iterator approach is expensive because taking a look for every item). You could then make the list immutable and only require a lock to get the pointer, and whenever the list is to be modified it could be copied. We would need a special stage at gst_init() time then, where the list can be modified (otherwise gst_init() will be very expensive as it adds one item at a time). But maybe this is all too complicated and we can just go with your patch :)
Created attachment 313607 [details] [review] registry: allow plugin and feature filter funcs to call registry API [alternative patch] [v2] Slightly different version that uses a stack-allocated array instead of a GList. We still have to traverse the list of all plugins or features though to ref all of them initially, even with first=true. I think that's probably okay, but let's wait a bit in case others have an opinion on this.
Created attachment 313891 [details] silly benchmark (drop into gstreamer/tests/benchmarks/)
Ran some stupid benchmarks out of curiosity (average run of 10000 cycles; always with first=false): -------------------- RPI2 (1336 features) -------------------- match-none match-all current 28us 838us list-copy 592us 1392us iterator 5382us 6158us ----------------------- core i7 (1347 features) ----------------------- match-none match-all current 7us 59us list-copy 36us 89us iterator 364us 405us Even on an rpi the impact is in the area of 0.5 milliseconds in the worst case scenario (full iteration). Since this is not something that's done a lot that strikes me as not something to worry about too much. Adding complicated new API for a task like this just to maintain a small performance advantage for something that's not super performance sensitive seems unnecessary to me. Also the iterator API is considerably slower than my proposed solution.
Go for it then :) The iterator is probably slower because of the locking once per item.
commit f02d09c362d119bf79a49275d1cb7a0c1f0f6bc4 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sat Oct 17 18:01:47 2015 +0100 registry: allow plugin and feature filter funcs to call registry API Don't keep the registry locked whilst iterating over the plugins or features with a filter function. This would deadlock if the callback tried to access the registry from the function. Instead, make a copy of the feature/plugin list and then filter it without holding the registry lock. This is still considerably faster than the alternative which would be to use a GstIterator. https://bugzilla.gnome.org/show_bug.cgi?id=756738