Bug 180839

Summary: Web Inspector: Canvas tab: throttle recording slider updates
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, ews-watchlist, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=181832
Bug Depends on:    
Bug Blocks: 173807, 175485    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Patch
none
Patch
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Patch
none
Patch
none
Patch none

Description Matt Baker 2017-12-14 14:07:51 PST
Summary:
Throttle recording slider updates. Rendering a 2D recording snapshot can be very time consuming, and dragging the slider quickly is unusable when viewing large recordings. This will help to mitigate the problem, but will require the following related patch to see bigger gains:

Web Inspector: Canvas tab: improve UI responsiveness when creating recording snapshots	
https://bugs.webkit.org/show_bug.cgi?id=180838
Comment 1 Radar WebKit Bug Importer 2017-12-14 14:08:29 PST
<rdar://problem/36057849>
Comment 2 Matt Baker 2017-12-14 14:16:52 PST
Created attachment 329403 [details]
Patch
Comment 3 Joseph Pecoraro 2017-12-14 15:51:03 PST
Comment on attachment 329403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329403&action=review

> Source/WebInspectorUI/ChangeLog:11
> +        executes immediately on the leading and trailing edges, and at most

"Immediately on the leading edge" yes, but I don't think it can be "immediate" on the trailing edge, it is just guaranteed on the trailing edge.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1490
> +        value: function(delay)

Style: `value(delay)` shorthand syntax works fine here.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1496
> +            if (!this[throttleProxySymbol]) {
> +                let lastFireTime = NaN;
> +                let mostRecentArguments = null;
> +
> +                this[throttleProxySymbol] = new Proxy(this, {

This means that the first call to throttle sets the delay that will be used forever.

r- since this appears to be different from debounce.

`debounce(delay)` returns a new proxy each time, and that new proxy will apply `delay` to each subsequent sub access.

Here `throttle(delay)` is cached on the object and forever returned later on. So `throttle(100); throttle(200)` return the same value (a 100 ms throttle).

I think we should avoid the caching and return a new proxy each time.

While it would be hard to write a non-flakey test for this behavior we should be able to test manually with something like:

    let startTime;
    let object = {
        test() {
            console.log("FIRE", (Date.now() - startTime) + "ms");
        }
    };
    
    setTimeout(() => {
        let throttle1 = object.throttle(20);
        startTime = Date.now();
        throttle1.test();
        setTimeout(() => { throttle1.test() }, 5);
        setTimeout(() => { throttle1.test() }, 12);
        setTimeout(() => { throttle1.test() }, 22);
        setTimeout(() => { throttle1.test() }, 32);
        // Expect:
        // FIRE: 0ms
        // FIRE: 20ms
        // FIRE: 40ms
    }, 0);
    
    setTimeout(() => {
        let throttle2 = object.throttle(10);
        startTime = Date.now();
        throttle1.test();
        setTimeout(() => { throttle1.test() }, 5);
        setTimeout(() => { throttle1.test() }, 12);
        setTimeout(() => { throttle1.test() }, 22);
        setTimeout(() => { throttle1.test() }, 32);
        // Expect:
        // FIRE: 0ms
        // FIRE: 10ms
        // FIRE: 20ms
        // FIRE: 30ms
        // FIRE: 40ms
    }, 100);

Where FIRE +/- 1ms off. Right now I'd expect the second case to be incorrectly throttled at 20ms instead of 10ms.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1534
> +        value: function()

Style: `value()`

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:546
> +        this.throttle(250).dispatchEventToListeners(WI.RecordingContentView.Event.RecordingActionIndexChanged, {index});

Note that this is exactly what I think is broken right now. There are two throttles on the same object, so I think this throttle is actually 200ms instead of a 250ms.

That said, I don't think you want the throttle in both places. (they are competing over throttles for no good reason). Just one place should be enough, whichever you choose.

> LayoutTests/inspector/unit-tests/throttle.html:14
> +            const expectedDelay = 20;

We should probably increase 20. Debug builds on the bots often struggle to meet timeouts that are less than 100ms.

If we increase this to 50ms that might prevent flakiness.

Another possibility is to just count calls and compare timestamps to be greater than some minimum. And the result should be: (no more than N calls, and less than N ms). But I think what you have is fine.
Comment 4 Joseph Pecoraro 2017-12-14 15:53:06 PST
> (no more than N calls, and less than N ms)

I meant: (no more than N calls, and no less than M ms)
Comment 5 Matt Baker 2017-12-14 16:07:47 PST
Comment on attachment 329403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329403&action=review

>> Source/WebInspectorUI/ChangeLog:11
>> +        executes immediately on the leading and trailing edges, and at most
> 
> "Immediately on the leading edge" yes, but I don't think it can be "immediate" on the trailing edge, it is just guaranteed on the trailing edge.

Will fix the wording here, trailing is definitely not immediate.

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1496
>> +                this[throttleProxySymbol] = new Proxy(this, {
> 
> This means that the first call to throttle sets the delay that will be used forever.
> 
> r- since this appears to be different from debounce.
> 
> `debounce(delay)` returns a new proxy each time, and that new proxy will apply `delay` to each subsequent sub access.
> 
> Here `throttle(delay)` is cached on the object and forever returned later on. So `throttle(100); throttle(200)` return the same value (a 100 ms throttle).
> 
> I think we should avoid the caching and return a new proxy each time.
> 
> While it would be hard to write a non-flakey test for this behavior we should be able to test manually with something like:
> 
>     let startTime;
>     let object = {
>         test() {
>             console.log("FIRE", (Date.now() - startTime) + "ms");
>         }
>     };
>     
>     setTimeout(() => {
>         let throttle1 = object.throttle(20);
>         startTime = Date.now();
>         throttle1.test();
>         setTimeout(() => { throttle1.test() }, 5);
>         setTimeout(() => { throttle1.test() }, 12);
>         setTimeout(() => { throttle1.test() }, 22);
>         setTimeout(() => { throttle1.test() }, 32);
>         // Expect:
>         // FIRE: 0ms
>         // FIRE: 20ms
>         // FIRE: 40ms
>     }, 0);
>     
>     setTimeout(() => {
>         let throttle2 = object.throttle(10);
>         startTime = Date.now();
>         throttle1.test();
>         setTimeout(() => { throttle1.test() }, 5);
>         setTimeout(() => { throttle1.test() }, 12);
>         setTimeout(() => { throttle1.test() }, 22);
>         setTimeout(() => { throttle1.test() }, 32);
>         // Expect:
>         // FIRE: 0ms
>         // FIRE: 10ms
>         // FIRE: 20ms
>         // FIRE: 30ms
>         // FIRE: 40ms
>     }, 100);
> 
> Where FIRE +/- 1ms off. Right now I'd expect the second case to be incorrectly throttled at 20ms instead of 10ms.

I didn't notice this about the debounce implementation the first time through. Will fix.

>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:546
>> +        this.throttle(250).dispatchEventToListeners(WI.RecordingContentView.Event.RecordingActionIndexChanged, {index});
> 
> Note that this is exactly what I think is broken right now. There are two throttles on the same object, so I think this throttle is actually 200ms instead of a 250ms.
> 
> That said, I don't think you want the throttle in both places. (they are competing over throttles for no good reason). Just one place should be enough, whichever you choose.

I only intended to throttle the event listener, not the dispatch. This was accidentally left in.

>> LayoutTests/inspector/unit-tests/throttle.html:14
>> +            const expectedDelay = 20;
> 
> We should probably increase 20. Debug builds on the bots often struggle to meet timeouts that are less than 100ms.
> 
> If we increase this to 50ms that might prevent flakiness.
> 
> Another possibility is to just count calls and compare timestamps to be greater than some minimum. And the result should be: (no more than N calls, and less than N ms). But I think what you have is fine.

I try to avoid huge delays so the test doesn't run longer than necessary, but didn't know the debug bots had so much trouble here. For now I'll just increase the timeout, but the alternative approach to this test is a good suggestion.
Comment 6 Joseph Pecoraro 2017-12-14 16:14:15 PST
> I try to avoid huge delays so the test doesn't run longer than necessary,
> but didn't know the debug bots had so much trouble here.

Yeah, long tests stink, but fortunately on the bots so many tests running in parallel doesn't make them hurt as badly.
Comment 7 Matt Baker 2017-12-15 13:30:14 PST
Created attachment 329513 [details]
Patch
Comment 8 Devin Rousso 2017-12-15 14:39:07 PST
Comment on attachment 329513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329513&action=review

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:178
> +        this._sliderElement.addEventListener("input", this.throttle(200)._sliderChanged.bind(this));

I actually think it might be better to throttle the `_generateContentCanvas*` calls, as this would only prevent lots of changes to the slider.  If the user decides to hold the up/down arrow when focused on the TreeOutline in the RecordingNavigationSidebarPanel, that could also trigger a huge number of updates within a short span.  I haven't tested this though, so I could be wrong.  Just a thought :)
Comment 9 EWS Watchlist 2017-12-15 15:46:44 PST
Comment on attachment 329513 [details]
Patch

Attachment 329513 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5677475

New failing tests:
webgl/1.0.2/conformance/context/context-release-with-workers.html
Comment 10 EWS Watchlist 2017-12-15 15:46:45 PST
Created attachment 329530 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Matt Baker 2017-12-18 14:01:45 PST
Created attachment 329679 [details]
Patch
Comment 12 Matt Baker 2018-01-05 11:49:44 PST
Created attachment 330565 [details]
Patch
Comment 13 EWS Watchlist 2018-01-05 14:13:05 PST
Comment on attachment 330565 [details]
Patch

Attachment 330565 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5945318

New failing tests:
inspector/unit-tests/throttle.html
Comment 14 EWS Watchlist 2018-01-05 14:13:07 PST
Created attachment 330582 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 15 Matt Baker 2018-01-08 14:58:18 PST
Created attachment 330741 [details]
Patch
Comment 16 Joseph Pecoraro 2018-01-10 13:15:12 PST
Comment on attachment 330741 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330741&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1485
> +    const throttleTimeoutSymbol = Symbol("throttle-timeout");

I really dislike how we've implemented both debounce and throttle to have this shared symbol. So two throttles on the same object end up smashing over each other. But that is not specific to your patch that is preexisting poor design.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:118
>              if (this.representedObject.type === WI.Recording.Type.Canvas2D)
> -                this._generateContentCanvas2D(index, actions, options);
> +                this.throttle(200)._generateContentCanvas2D(index, actions, options);
>              else if (this.representedObject.type === WI.Recording.Type.CanvasWebGL)
> -                this._generateContentCanvasWebGL(index, actions, options);
> +                this.throttle(200)._generateContentCanvasWebGL(index, actions, options);

r- This code doesn't appear to throttle anything.

Throttling only happens when you have a throttle proxy and invoke actions on that proxy many times:

    let throttler = this.thottle(200)
    throttle.action();
    throttle.action();
    throttle.action();

However this code creates a new throttle every time and makes 1 call on it, which will perform immediately every time.
Comment 17 Matt Baker 2018-01-10 14:10:48 PST
(In reply to Joseph Pecoraro from comment #16)
> Comment on attachment 330741 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330741&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1485
> > +    const throttleTimeoutSymbol = Symbol("throttle-timeout");
> 
> I really dislike how we've implemented both debounce and throttle to have
> this shared symbol. So two throttles on the same object end up smashing over
> each other. But that is not specific to your patch that is preexisting poor
> design.

Completely agree. We only use throttle/debounce in a few places, so probably not worth the effort right now.

> 
> > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:118
> >              if (this.representedObject.type === WI.Recording.Type.Canvas2D)
> > -                this._generateContentCanvas2D(index, actions, options);
> > +                this.throttle(200)._generateContentCanvas2D(index, actions, options);
> >              else if (this.representedObject.type === WI.Recording.Type.CanvasWebGL)
> > -                this._generateContentCanvasWebGL(index, actions, options);
> > +                this.throttle(200)._generateContentCanvasWebGL(index, actions, options);
> 
> r- This code doesn't appear to throttle anything.
> 
> Throttling only happens when you have a throttle proxy and invoke actions on
> that proxy many times:
> 
>     let throttler = this.thottle(200)
>     throttle.action();
>     throttle.action();
>     throttle.action();
> 
> However this code creates a new throttle every time and makes 1 call on it,
> which will perform immediately every time.

Oops! I forgot to update this after changing Object.throttle to not return a cached proxy.
Comment 18 Matt Baker 2018-01-10 14:11:20 PST
Created attachment 330960 [details]
Patch
Comment 19 Joseph Pecoraro 2018-01-10 15:12:05 PST
Comment on attachment 330960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330960&action=review

r=me

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:-122
> -    // Protected

Aren't shown/hidden protected though?

> LayoutTests/inspector/unit-tests/throttle.html:110
> +            let throttleProxy = object.throttle(delay);

I feel like this should be slightly less than the delay, like 20, in order to further prove that the throttle didn't fire after the verification timeout of 50ms.
Comment 20 Matt Baker 2018-01-10 16:01:08 PST
Comment on attachment 330960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330960&action=review

>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:-122
>> -    // Protected
> 
> Aren't shown/hidden protected though?

They aren't marked protected in the base class, and are called from outside ContentView so I think public makes the most sense. Currently we call things "protected" that should only be accessible to derived classes (like C++ protected) or a tightly-coupled external class (like C++ friend). Since both are just convention, I'm in favor of only using it to mean C++ protected.

>> LayoutTests/inspector/unit-tests/throttle.html:110
>> +            let throttleProxy = object.throttle(delay);
> 
> I feel like this should be slightly less than the delay, like 20, in order to further prove that the throttle didn't fire after the verification timeout of 50ms.

Sounds good to me.
Comment 21 Matt Baker 2018-01-10 16:06:58 PST
Created attachment 330983 [details]
Patch
Comment 22 WebKit Commit Bot 2018-01-10 19:36:34 PST
Comment on attachment 330983 [details]
Patch

Clearing flags on attachment: 330983

Committed r226755: <https://trac.webkit.org/changeset/226755>
Comment 23 WebKit Commit Bot 2018-01-10 19:36:36 PST
All reviewed patches have been landed.  Closing bug.