Bug 180839

<rdar://problem/36057849>

Created attachment 329403 [details] Patch

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.

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

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.

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

Created attachment 329513 [details] Patch

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

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

Created attachment 329679 [details] Patch

Created attachment 330565 [details] Patch

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

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

Created attachment 330741 [details] Patch

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.

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

Created attachment 330960 [details] Patch

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

Created attachment 330983 [details] Patch

Comment on attachment 330983 [details] Patch Clearing flags on attachment: 330983 Committed r226755: <https://trac.webkit.org/changeset/226755>

All reviewed patches have been landed. Closing bug.