* SUMMARY Filtering huge data grids should yield occasionally so the UI remains responsive. Ideally the Inspector would have a generic mechanism whereby a task that processes a large number of items (such as data grid nodes) could yield control back to the runloop after a specified amount of time, before resuming processing.
<rdar://problem/26282898>
Created attachment 278914 [details] [Patch] Proposed Fix
Comment on attachment 278914 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278914&action=review r=me, but we really should have tests that do basic coverage of the new class. > Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:26 > +WebInspector.BatchedTaskController = class BatchedTaskController extends WebInspector.Object This could really use some tests! - tasks that don't end up yielding - tasks that do end up yielding - stopped by user and used again > Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:32 > + console.assert(delegate && typeof delegate.batchedTaskControllerProcessItem === "function", "Missing delegate implementing 'batchedTaskControllerProcessItem'"); Nit: console.assert(timeLimitPerBatch > 0) > Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:50 > + if (this._processing) > + return; Nit: console.assert(!this._processing); Otherwise seems we should just ignore the new tasks in this case so there might be a silent error somewhere if this is called multiple times. And therefore probably a getter for processing would make sense. > Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:100 > + while (!this._batchGenerator.next().done) { > + this._batchTimeoutIdentifier = setTimeout(() => { this._processBatch(); }, 0); > + return; > + } Nit: "if" instead of "while" would be clearer. > Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:130 > +}; This class is really clean, nice!
Comment on attachment 278914 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278914&action=review >> Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:26 >> +WebInspector.BatchedTaskController = class BatchedTaskController extends WebInspector.Object > > This could really use some tests! > > - tasks that don't end up yielding > - tasks that do end up yielding > - stopped by user and used again There are tests a plenty, I just ran webkit-patch from the wrong directory.
Comment on attachment 278914 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278914&action=review Very cool! Nice use of generators. > Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:126 > + let stoppedByUser = this._stoppedByUser; stoppedByClient? The "user" might not be involved. > Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1749 > + this._filterDidModifyNodeInCurrentBatch = false; You should reset this before filtering starts too, to be on the safe side.
Created attachment 278949 [details] [Patch] Proposed Fix
Comment on attachment 278914 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278914&action=review >> Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:126 >> + let stoppedByUser = this._stoppedByUser; > > stoppedByClient? The "user" might not be involved. processingCancelled?
Created attachment 278950 [details] [Patch] Proposed Fix
Comment on attachment 278950 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278950&action=review > Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:91 > + this._processingCancelled = true; _processingStopped?
Comment on attachment 278950 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278950&action=review >> Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:91 >> + this._processingCancelled = true; > > _processingStopped? I think cancelled makes it clear that the operation was stopped before finishing.
Comment on attachment 278950 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278950&action=review >>> Source/WebInspectorUI/UserInterface/Controllers/BatchedTaskController.js:91 >>> + this._processingCancelled = true; >> >> _processingStopped? > > I think cancelled makes it clear that the operation was stopped before finishing. Then maybe the method should match? cancelProcessing()
Created attachment 278980 [details] [Patch] Proposed Fix
Went with startTask/cancelTask.
Comment on attachment 278980 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=278980&action=review I have an idea for how to make this even cleaner, will post a patch to save you some time. > Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1734 > + batchedTaskControllerProcessItem(node) Delegate methods need to pass the delegating instance as the first argument.
Created attachment 279045 [details] [Patch] Proposed Fix
I think this patch should be considered as-is. Adding support for multiple tasks would require a token to be returned from startTask, and passed to delegate methods. The same thing can be accomplished by instantiating multiple YieldableTask objects.
Created attachment 279065 [details] Proposed Fix v2
Comment on attachment 279065 [details] Proposed Fix v2 I went a slightly different direction here, because I think it will be easier to use: - YieldableTasks don't require a controller, but still use the delegate approach - They cannot be reused - The items iterator is passed at construction time Also addressed some style and correctness things: - Don't use 'batch' anywhere unless referring to something coalesced, like events - Check to see if cancelled before *and* after processing an item - delegate methods pass the delegating object as first parameter - try naming generator functions as createIteratorForXXX, use call instead of bind. - Delay notifying delegate about finishing when task is cancelled This gets us multiple tasks per object for free, more or less, in case we need to do that in the future. It also simplifies the lifetime of tasks so they cannot be accidentally leaked or reused.
(In reply to comment #16) > I think this patch should be considered as-is. Adding support for multiple > tasks would require a token to be returned from startTask, and passed to > delegate methods. The same thing can be accomplished by instantiating > multiple YieldableTask objects. Right. The only thing I needed to change to get this was the name, really. Also, passing the delegating object in case there is more than one task. The rest is pretty much the same.
Comment on attachment 279065 [details] Proposed Fix v2 View in context: https://bugs.webkit.org/attachment.cgi?id=279065&action=review > Source/WebInspectorUI/UserInterface/Base/YieldableTask.js:72 > + function* createIteratorForProcessingItems() I think "createIteratorFor" is confusing. I would call this "processingItemsIterator". > Source/WebInspectorUI/UserInterface/Base/YieldableTask.js:93 > + this._willYield(returnedItems, elapsedTime); > + yield; Newline before? > Source/WebInspectorUI/UserInterface/Base/YieldableTask.js:106 > + this._pendingItemsIterator = createIteratorForProcessingItems.call(this); I suppose this is why you used create. I am surprised call() works… maybe I need to read up on generators. > Source/WebInspectorUI/UserInterface/Base/YieldableTask.js:137 > + _willYield(processedItems, elapsedTime) _didYield? It is confusing since this calls yieldableTaskDidYield. > Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1744 > + function *createIteratorForNodesToBeFiltered() I would call this nodesToBeFilteredIterator. > Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1760 > + let options = { > + workInterval: 100, > + }; Inline it. Having it broken out makes me hate options arguments.
Comment on attachment 279065 [details] Proposed Fix v2 Attachment 279065 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1333465 New failing tests: inspector/unit-tests/yieldable-task.html
Created attachment 279072 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 279065 [details] Proposed Fix v2 Attachment 279065 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1333569 New failing tests: inspector/unit-tests/yieldable-task.html
Created attachment 279074 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #19) > (In reply to comment #16) > > I think this patch should be considered as-is. Adding support for multiple > > tasks would require a token to be returned from startTask, and passed to > > delegate methods. The same thing can be accomplished by instantiating > > multiple YieldableTask objects. > > Right. The only thing I needed to change to get this was the name, really. > Also, passing the delegating object in case there is more than one task. The > rest is pretty much the same. Both of those items were addressed in the patch under comment 15.
(In reply to comment #25) > (In reply to comment #19) > > (In reply to comment #16) > > > I think this patch should be considered as-is. Adding support for multiple > > > tasks would require a token to be returned from startTask, and passed to > > > delegate methods. The same thing can be accomplished by instantiating > > > multiple YieldableTask objects. > > > > Right. The only thing I needed to change to get this was the name, really. > > Also, passing the delegating object in case there is more than one task. The > > rest is pretty much the same. > > Both of those items were addressed in the patch under comment 15. Ugh! For some reason I didn't get any emails about all the subsequent patches that you wrote after the first one. Sorry for wasting your time; I thought you were working on other things at the time.
(In reply to comment #24) > Created attachment 279074 [details] > Archive of layout-test-results from ews103 for mac-yosemite > > The attached test failures were seen while running run-webkit-tests on the > mac-ews. > Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5 Looks like the DidFinish callback is getting called twice, probably because more than one place is trying to send that during teardown.
Comment on attachment 279065 [details] Proposed Fix v2 View in context: https://bugs.webkit.org/attachment.cgi?id=279065&action=review >> Source/WebInspectorUI/UserInterface/Base/YieldableTask.js:106 >> + this._pendingItemsIterator = createIteratorForProcessingItems.call(this); > > I suppose this is why you used create. I am surprised call() works… maybe I need to read up on generators. Invoking a generator function creates a new iterator instance with its own state. This is basically a factory method. Once we have more examples in the code base, we should have a team discussion about the style to use for generators. Ref: "A generator is a special type of function that works as a factory for iterators." https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Iterators_and_Generators > Source/WebInspectorUI/UserInterface/Base/YieldableTask.js:120 > + setTimeout(() => { this._didFinish(); }, 0); It turns out this is unnecessary and responsible for the test flakiness. I will remove it before landing. >> Source/WebInspectorUI/UserInterface/Base/YieldableTask.js:137 >> + _willYield(processedItems, elapsedTime) > > _didYield? It is confusing since this calls yieldableTaskDidYield. There's no way to notify after the actual yield as the iterator's context will be suspended. So we set up state as if the yield happened, and then notify.
Created attachment 279150 [details] For Landing
Comment on attachment 279150 [details] For Landing Attachment 279150 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1337857 Number of test failures exceeded the failure limit.
Created attachment 279155 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Committed r201047: <http://trac.webkit.org/changeset/201047>
The new test is somewhat flaky: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Funit-tests%2Fyieldable-task.html