We made GMainLoopSource thread safe, but in most of the cases we know the sources is used by a single thread, which has an impact in the performance (mutex, GCancellable, etc.). We can move the thread safe part to a new derived class, leaving the basic common implementation in GMainLoopSource, and explicitly using the thread safe version when needed.
Created attachment 239408 [details] Patch
Attachment 239408 [details] did not pass style-queue: ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:63: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:98: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 6 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239411 [details] Add new file to EFL build too
Attachment 239411 [details] did not pass style-queue: ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:63: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.cpp:98: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 6 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
The implementation looks OK. Still, in my experience the main bottleneck in GMainLoopSource is the constant creation and destruction of GSource objects. I'd prefer to see that addressed first, and work on thread-safety simplification from there. I'll dump my current approach towards that either in a bug or in a pastebin, and we can discuss it from there.
(In reply to comment #5) > The implementation looks OK. Thanks for looking at it. > Still, in my experience the main bottleneck in GMainLoopSource is the constant creation and destruction of GSource objects. I'd prefer to see that addressed first, and work on thread-safety simplification from there. Constant creation and destruction of GSources is the normal way glib main loop works, so I'm surprised that causes problems. g_idle and g_timeout functions in glib always create a new GSource, so there shouldn't be differences in that regard between using glib functions directly or GMainLoopSource. I haven't measured but I would say that creating a GCancellable (which is a GObject), and the mutex lock/unlock on every schedule is a lot more demanding than creating and destroying the sources. > I'll dump my current approach towards that either in a bug or in a pastebin, and we can discuss it from there. Ok, I think any optimization in the way glib sources work should be done in glib itself for the benefit of everybody.
(In reply to comment #6) > (In reply to comment #5) > > Still, in my experience the main bottleneck in GMainLoopSource is the constant creation and destruction of GSource objects. I'd prefer to see that addressed first, and work on thread-safety simplification from there. > > Constant creation and destruction of GSources is the normal way glib main loop works, so I'm surprised that causes problems. g_idle and g_timeout functions in glib always create a new GSource, so there shouldn't be differences in that regard between using glib functions directly or GMainLoopSource. > I haven't measured but I would say that creating a GCancellable (which is a GObject), and the mutex lock/unlock on every schedule is a lot more demanding than creating and destroying the sources. > I profiled the current situation, your patch and my work in progress. The test case is a simple loop via setTimeout(loop, 1), which is handled by the shared timer is throttled to 250 iterations per second. Current state: http://people.igalia.com/zdobersek/gms-current.png Horrible, spends 24% of overall time under GMainLoopSource::scheduleAfterDelay(). That's a quarter of runtime to create and attach ~250 GSource objects per second. Your patch: http://people.igalia.com/zdobersek/gms-optional-thread-safety.png Already does wonders, dropping the time under GMainLoopSource::scheduleAfterDelay() to a bit under 14% of overall time. The simple source WIP: http://people.igalia.com/zdobersek/gms-simplified.png Uses a single GSource object, and it shows. Scheduling the callback dispatch (which at this point is just setting the ready time on the source) only takes 4% of the runtime, and there's basically no GObject churn. Comparing the time spent under the GSource dispatch function, it goes from ~52% down to ~27% and further down to ~14%. There's no problem in constant creation and destruction of GSource objects in terms of correctness, but it's disturbing in the terms of performance. Feel free to proceed with this patch, it actually already improves things more than I would've expected. But I want to address these high-intensity timers that can hinder performance, starting probably with the shared timer and the flush timer in LayerTreeHostGtk. The simple implementation I outlined in bug #137513 does that, but needs discussion and some sort of approval.
Thanks for measuring, as I said both optimizations are compatible. My point was that the persistent source optimization could be applied anyway, but it could be done using the same API, and transparently to the user. Could you measure again with this patch and the one I attached to bug #137513?
(In reply to comment #8) > Thanks for measuring, as I said both optimizations are compatible. My point was that the persistent source optimization could be applied anyway, but it could be done using the same API, and transparently to the user. Could you measure again with this patch and the one I attached to bug #137513? Here's the profile: http://people.igalia.com/zdobersek/gms-optional-thread-safety-simplified.png Not really better than without the persistent source implementation, pretty much the same actually. Though the time is now spent in additional mutex locking via g_source_set_ready_time(), g_main_context_get_default() and GSource ref-unrefing, and in string comparison.
(In reply to comment #9) > (In reply to comment #8) > > Thanks for measuring, as I said both optimizations are compatible. My point was that the persistent source optimization could be applied anyway, but it could be done using the same API, and transparently to the user. Could you measure again with this patch and the one I attached to bug #137513? > > Here's the profile: > http://people.igalia.com/zdobersek/gms-optional-thread-safety-simplified.png > > Not really better than without the persistent source implementation, pretty much the same actually. Though the time is now spent in additional mutex locking via g_source_set_ready_time(), g_main_context_get_default() and GSource ref-unrefing, and in string comparison. hmm, g_source_set_ready_time() is called mostly in the same places than your patch, so that shouldn't make any difference. The string comparison was to check if the source name changed, to avoid dealloc/alloc and mutex lock/unlock that happens in g_source_set_name. I tried to make it transparent for the users, but if we want to avoid all those checks, I guess we need specific API for persistent sources, or even a different class like in your patch. Thanks again for measuring
Comment on attachment 239411 [details] Add new file to EFL build too View in context: https://bugs.webkit.org/attachment.cgi?id=239411&action=review > Source/WTF/wtf/gobject/GMutexLocker.h:78 > + return; Hmm this makes the template useless, because the recursive mutexes will act as normal ones, won't they? > Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.h:33 > +typedef struct _GRecMutex GRecMutex; What do we need this for?
Comment on attachment 239411 [details] Add new file to EFL build too View in context: https://bugs.webkit.org/attachment.cgi?id=239411&action=review >>> Source/WTF/wtf/gobject/GMutexLocker.h:78 >>> + return; >> >> Hmm this makes the template useless, because the recursive mutexes will act as normal ones, won't they? > > Hmm this makes the template useless, because the recursive mutexes will act as normal ones, won't they? The idea is that the same GMutexLocker object shouldn't lock a mutex already locked, but two different GMutexLocker objects, wrapping the same GRecMutex should be allowed. Which is the case I'm covering here, schedule calls cancel, both locking the mutex, but using different GMutexLocker objects. >>> Source/WTF/wtf/gobject/GThreadSafeMainLoopSource.h:33 >>> +typedef struct _GRecMutex GRecMutex; >> >> What do we need this for? > > What do we need this for? It's the forward declaration of GRecMutex, needed by the m_mutex memeber that is a GRecMutex, to avoid including glib.h in the header.
Comment on attachment 239411 [details] Add new file to EFL build too View in context: https://bugs.webkit.org/attachment.cgi?id=239411&action=review >>>> Source/WTF/wtf/gobject/GMutexLocker.h:78 >>>> + return; >>> >>> Hmm this makes the template useless, because the recursive mutexes will act as normal ones, won't they? >> >> Hmm this makes the template useless, because the recursive mutexes will act as normal ones, won't they? > > The idea is that the same GMutexLocker object shouldn't lock a mutex already locked, but two different GMutexLocker objects, wrapping the same GRecMutex should be allowed. Which is the case I'm covering here, schedule calls cancel, both locking the mutex, but using different GMutexLocker objects. Right. Too many mutexes and lockers :). Couldn't we achieve the same by using the _try_ variants in MutexWrapper object?
(In reply to comment #14) > (From update of attachment 239411 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239411&action=review > > >>>> Source/WTF/wtf/gobject/GMutexLocker.h:78 > >>>> + return; > >>> > >>> Hmm this makes the template useless, because the recursive mutexes will act as normal ones, won't they? > >> > >> Hmm this makes the template useless, because the recursive mutexes will act as normal ones, won't they? > > > > The idea is that the same GMutexLocker object shouldn't lock a mutex already locked, but two different GMutexLocker objects, wrapping the same GRecMutex should be allowed. Which is the case I'm covering here, schedule calls cancel, both locking the mutex, but using different GMutexLocker objects. > > Right. Too many mutexes and lockers :). Couldn't we achieve the same by using the _try_ variants in MutexWrapper object? No, try_lock is in case it's locked by *another* thread, to not block. But if locked by the same thread, in case of non-recursive mutex, the behaviour is undefined.
Committed r174632: <http://trac.webkit.org/changeset/174632>