showModalDialog fix creates risk of never returning from RunLoop::performWork, potentially starving other event sources The fix in question was for bug 89590 and can be seen at http://trac.webkit.org/changeset/120879 The scenario is as follows: -RunLoop::performWork() is invoked and there is 1 function on the queue -Handling that one function results in 1 more function being placed on the queue -Handling that one results in yet another being placed on the queue -Ad infinitum Therefore that invocation of performWork() never returns and all other event sources are blocked. Before r120879 this could never happen because we only ever did the amount of work that was in the queue when performWork() was invoked. Any additional work that was enqueued would have to wait until the next time performWork() was invoked which would be after other event sources had a chance to spin. We can approximate that same behavior with a simple count. Patch is coming up. In radar as <rdar://problem/11718988>
Created attachment 148832 [details] Patch v1 - Proposed fix This patch implements a rule where we only perform the number of functions in the queue at the time performWork was called. Additional functions will wait until the next time performWork is called. This very closely approximates our behavior before http://trac.webkit.org/changeset/120879. I originally re-wrote the loop to be a for loop but that required acquiring the lock before the for loop *just* to get the function count. By far the common case in RunLoop::performWork is that it is handling 1 function. So the for-loop solution would've required acquiring and releasing the lock twice just to perform one function. Having this simple branch inside the loop seems much more efficient than having that addition lock acquisition in the common case of 1 function, but I'm open to input on this.
Attachment 148832 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/RunLoop.cpp:82: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #1) > Created an attachment (id=148832) [details] > By far the common case in RunLoop::performWork is that it is handling 1 function. So the for-loop solution would've required acquiring and releasing the lock twice just to perform one function. > > Having this simple branch inside the loop seems much more efficient than having that addition lock acquisition in the common case of 1 function, but I'm open to input on this. I thought of a better way. New patch coming.
Created attachment 148837 [details] Patch v2 - Avoid taking the lock twice for the common case *and* avoid the branch comparison in Patch v1
Comment on attachment 148837 [details] Patch v2 - Avoid taking the lock twice for the common case *and* avoid the branch comparison in Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=148837&action=review I know Anders will review too. Looks fine to me, thought. > Source/WebCore/platform/RunLoop.cpp:72 > + // we guarantee that we will occassionally return from the run loop and other event sources will have Typo: occasionally has only one s
Comment on attachment 148837 [details] Patch v2 - Avoid taking the lock twice for the common case *and* avoid the branch comparison in Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=148837&action=review > Source/WebCore/platform/RunLoop.cpp:90 > + for (size_t functionsHandled = 1; functionsHandled < functionsToHandle; functionsHandled++) { pre, not post, plz 2 fix
(In reply to comment #6) > (From update of attachment 148837 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148837&action=review > > > Source/WebCore/platform/RunLoop.cpp:90 > > + for (size_t functionsHandled = 1; functionsHandled < functionsToHandle; functionsHandled++) { > > pre, not post, plz 2 fix Definitely fixed locally. Waiting to land until after Anders has also taken a look
http://trac.webkit.org/changeset/120954