GNOME Bugzilla – Bug 593284
basertppayloader takes time in instance init
Last modified: 2009-09-01 08:42:27 UTC
Created attachment 141845 [details] [review] Use g_random_int* instead of g_rand_int* g_rand_new is called 3 times during instance init, using g_random_int* makes it faster (although it takes the global GRand lock).
I wonder why those GRands are in the instance struct at all *sigh* There should be a FIXME 0.11 comment to remove them in 0.11 ;) The performance improvement is caused by the fact, that accessing /dev/urandom (i.e. doing some kind of IO) is more expensive than simply locking and unlocking a mutex, right?
Yeah, but it really depends on the contention on that mutex. The biggest performance problem however is that g_rand_new reads way too much from /dev/urandom. A fix for glib has been comitted to git today (http://bugzilla.gnome.org/show_bug.cgi?id=593232) but that won't help until it's released and in use everywhere.
I didn't quite realise these were exposed in the public header when I made my comments on IRC the other day. I don't think we can/should just remove them like you propose to, and if this goes into the next GLib release anyway, maybe it's best to keep things as they're now instead of introducing locking contention. If you still think we need to do something, how about: - #if !GLIB_VERSION_CHECK(2,21,6) GST_CAT_DEBUG (GST_CAT_PERFORMANCE, "slow g_rand_new(), update your glib"); #endif - using just one GRand instance instead of three?
The right thing to do is to go for one GRand and a warning and then wait for a glib upgrade.
I don't think using one GRand improves anything if we still must create all three GRands, it's g_rand_new that is slow. Note that I didn't remove the GRand pointers, just removed the initialisation os them. So the struct is unchanged.
one GRand is potentially dangerous too when a subclass would use the '3' GRand from different threads it would now explode. another option is to have the subclass set a class variable to tell the base class that it's aware that the GRands are gone.
Or use g_rand_new_with_seed[_array](), and create the seed manually from /dev/urandom or g_random_int().
Created attachment 142070 [details] [review] Init internal GRands with seed, but use global GRand Changed the patch to create the internal GRands with weaker, but faster, seeds. It still uses the global GRand and the internal seeds are marked for removal in 0.11.
(In reply to comment #8) > Created an attachment (id=142070) [details] > Init internal GRands with seed, but use global GRand > > Changed the patch to create the internal GRands with weaker, but faster, seeds. > It still uses the global GRand and the internal seeds are marked for removal in > 0.11. But when you're creating your own GRand instances anyway you could as well use them in instance_init ;) Apart from that this looks good IMHO
I wanted to use the global GRand because it has a better seed (16 bytes) compared to the internal ones (4 bytes).
commit ec91d508af9fb359ebab7240497bd043d84d652e Author: Jonas Holmberg <jonas.holmberg@axis.com> Date: Tue Sep 1 10:39:52 2009 +0200 basertppayload: Make instance init faster by not reading /dev/urandom 3 time ... which is the default seed when creating a new GRand. Because GLib in older versions used buffered IO this would take a lot of time. Instead use the global GRand for getting random numbers and keep the three instance GRand for backward compatibility with a simple seed. Fixes bug #593284.