Bug 157702 – Web Inspector: Filtering huge data grids should yield occasionally so the UI remains responsive

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* 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.

[Radar WebKit Bug Importer]

<rdar://problem/26282898>

[Matt Baker]

Created attachment 278914 [details]
[Patch] Proposed Fix

[Joseph Pecoraro]

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!

[Matt Baker]

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.

[Timothy Hatcher]

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.

[Matt Baker]

Created attachment 278949 [details]
[Patch] Proposed Fix

[Matt Baker]

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?

[Matt Baker]

Created attachment 278950 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

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?

[Matt Baker]

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.

[Timothy Hatcher]

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()

[Matt Baker]

Created attachment 278980 [details]
[Patch] Proposed Fix

[Matt Baker]

Went with startTask/cancelTask.

[Blaze Burg]

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.

[Matt Baker]

Created attachment 279045 [details]
[Patch] Proposed Fix

[Matt Baker]

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.

[Blaze Burg]

Created attachment 279065 [details]
Proposed Fix v2

[Blaze Burg]

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.

[Blaze Burg]

(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.

[Timothy Hatcher]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Matt Baker]

(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.

[Blaze Burg]

(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.

[Blaze Burg]

(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.

[Blaze Burg]

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.

[Blaze Burg]

Created attachment 279150 [details]
For Landing

[Build Bot]

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.

[Build Bot]

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

[Blaze Burg]

Committed r201047: <http://trac.webkit.org/changeset/201047>

[Alexey Proskuryakov]

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