Web Inspector: Canvas tab: throttle recording slider updates

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

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

[Radar WebKit Bug Importer]

<rdar://problem/36057849>

[Matt Baker]

Created attachment 329403 [details]
Patch

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

> (no more than N calls, and less than N ms)

I meant: (no more than N calls, and no less than M ms)

[Matt Baker]

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.

[Joseph Pecoraro]

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

[Matt Baker]

Created attachment 329513 [details]
Patch

[Devin Rousso]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Matt Baker]

Created attachment 329679 [details]
Patch

[Matt Baker]

Created attachment 330565 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Matt Baker]

Created attachment 330741 [details]
Patch

[Joseph Pecoraro]

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.

[Matt Baker]

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

[Matt Baker]

Created attachment 330960 [details]
Patch

[Joseph Pecoraro]

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.

[Matt Baker]

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.

[Matt Baker]

Created attachment 330983 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 330983 [details]
Patch

Clearing flags on attachment: 330983

Committed r226755: <https://trac.webkit.org/changeset/226755>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.