Bug 194721

Created attachment 362178 [details] Patch

Comment on attachment 362178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362178&action=review I like the direction you're going! Proxies made stacktraces hard to use. > Source/WebInspectorUI/UserInterface/Base/Debouncer.js:82 > + queueMicrotask(() => { Today I learned! https://www.chromestatus.com/feature/5111086432911360

Comment on attachment 362178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362178&action=review > Source/WebInspectorUI/UserInterface/Base/Debouncer.js:26 > +class Debouncer I'd encourage a description at the top of this file, much like we do for *Chart.js files that explains the problem this solves, how to use it, and so on. For a Base class like this, it would be nice! > Source/WebInspectorUI/UserInterface/Base/Debouncer.js:42 > + force() Other possible names `fireNow`, `runNow`, `invokeNow`? > Source/WebInspectorUI/UserInterface/Base/Throttler.js:26 > +class Throttler I'd encourage a description at the top of this file, much like we do for *Chart.js files that explains the problem this solves, how to use it, and so on. For a Base class like this, it would be nice! > Source/WebInspectorUI/UserInterface/Base/Throttler.js:68 > + let remaining = this._delay - (Date.now() - this._lastFireTime); > + if (remaining <= 0) { > + this._execute(); > + return; > + } > + > + if (this._timeoutIdentifier) > + return; I find this rather difficult to read. It is setting a timer only for the remainder of the delay (weird), it is checking if timeoutIdentifier even though it already cancelled above. I would expect a Throttler to always start a repeating timer the first time someone asks it to fire and should stop itself when it finally fires and nobody tickled it. So I'd expect: constructor(callback, delay) { this._callback = callback; this._delay = delay; this._lastArguments = []; this._shouldContinue = false; this._timeoutIdentifier = undefined; } force() { this.cancel(); this._lastArguments = arguments; this.start(); } fire() { this._lastArguments = arguments; if (!this._timeoutIdentifier) { this._start(); return; } this._shouldContinue = true; } cancel() { this._lastArguments = []; this._shouldContinue = false; if (this._timeoutIdentifier) { clearTimeout(this._timeoutIdentifier); this._timeoutIdentifier = undefined; } } // Private _executeCallback() { let args = this._lastArguments; this._lastArguments = []; this._shouldContinue = false; this.callback.apply(undefined, args); } _start() { console.assert(!this._timeoutIdentifier); console.assert(!this._shouldContinue); this._timeoutIdentifier = setInterval(() => { this._timerFired(); }, this._delay); this._executeCallback(); } _timerFired() { console.assert(this._timeoutIdentifier); if (this._shouldContinue) this._executeCallback(); else this.cancel(); } The interesting thing here is: • force() does a cancel to completely reset the state of the Throttler and then starts it again. • a setInterval is always running and each time it fires it says "should I continue or stop" This seems way simpler to me at the cost of always running a timer (which I still think is the right thing to do). This can also be written with a setTimeout instead of a setInterval, but I don't think that actually changes anything important.

(In reply to Joseph Pecoraro from comment #3) > > Source/WebInspectorUI/UserInterface/Base/Throttler.js:68 > > + let remaining = this._delay - (Date.now() - this._lastFireTime); > > + if (remaining <= 0) { > > + this._execute(); > > + return; > > + } > > + > > + if (this._timeoutIdentifier) > > + return; > > I find this rather difficult to read. It is setting a timer only for the remainder of the delay (weird), it is checking if timeoutIdentifier even though it already cancelled above. That was a mistake on my part. We shouldn't be calling `cancel`. The reason it uses the remainder is so that it doesn't have to call `setTimeout` if it's only executed once. It only creates the timer on the second call (within `delay` amount of time), at which point some time has already passed, which should be taken into account when throttling. > I would expect a Throttler to always start a repeating timer the first time someone asks it to fire and should stop itself when it finally fires and nobody tickled it. I don't like the idea of using `setInterval`, as it involves an additional call after `callback` gets executed to actually cancel the interval. > This seems way simpler to me at the cost of always running a timer (which I still think is the right thing to do). I disagree. It involves more code (literally, the number of lines is greater) and has the "side effect" of keeping the timer alive one iteration longer than necessary.

Created attachment 362193 [details] Patch

> > I would expect a Throttler to always start a repeating timer the first time someone asks it to fire and should stop itself when it finally fires and nobody tickled it. > > I don't like the idea of using `setInterval`, as it involves an additional > call after `callback` gets executed to actually cancel the interval. The whole point of a Throttler class is to throttle something that is likely called multiple times. Timers are expected. > > This seems way simpler to me at the cost of always running a timer (which I still think is the right thing to do). > > I disagree. It involves more code (literally, the number of lines is > greater) and has the "side effect" of keeping the timer alive one iteration > longer than necessary. Logically and semantically I see this as much simpler. Throttler simply takes a callback `callback` and calls it at no more than a given frequency of `delay`. This can be literally translated into code as `call callback` and `setInterval(delay)`. Playing games with sometimes setting a timeout (which is an expected case) at different remainder times feels like complexity for no concrete benefit. But for something that is less than 100 lines it seems unimportant.

Comment on attachment 362193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362193&action=review Awesome! I can't wait to use these. Reading the tests hurts my eyes. Maybe we should rename the file so that the diff is cleaner? Or maybe I'll just apply it locally and review like that. > Source/WebInspectorUI/UserInterface/Base/Debouncer.js:28 > +// Debouncer can be used to wrap a function and "delay" it's execution for a certain amount of time > +// after it's most recent call. As an example, calling `delayForTime(10)` will prevent `callback` > +// from firing until 10ms after the most recent time it was called. I expected something more like: // Debouncer wraps a function and continues to delay its invocation as long as // clients continue to delay its firing. The most recent delay call overrides // previous calls. Delays may be timeouts, animation frames, or microtasks. // // Example: // // let debouncer = new Debouncer(() => { this.refresh() }); // elem.onkeydown = (event) => { debouncer.delayForTime(100); } // // Will ensure `refresh` will not happen until no keyevent has happened in 100ms: // // 0ms 100ms 200ms 300ms 400ms // time: |-------------|-------------|-------------|-------------| // delay: ^ ^ ^ ^ ^ ^ ^ ^ // refreshes: * (1) > Source/WebInspectorUI/UserInterface/Base/Throttler.js:28 > +// Throttler can be used to wrap a function and ensure that it won't be called more than once every > +// `delay`ms. Calling `fire` twice in a row will execute `callback` synchronously on the first call > +// and asynchronously `delay`ms after the first call (scheduled by the second call). I expected something more like: // Throttler wraps a function and ensures it doesn't get called more than once every `delay` ms. // The first fire will always trigger synchronous, and subsequent calls that need to be throttled // will happen later asynchronously. // // Example: // // let throttler = new Throttler(250, () => { this.refresh() }); // elem.onkeydown = (event) => { throttler.fire(); } // // Will ensure `refresh` happens at a frequency of 250ms no matter how often `fire` is called: // // 0ms 250ms 500ms // time: |---------------------------|---------------------------| // fire: ^ ^ ^ ^ ^ ^ ^ ^ // refreshes: * (1) * (2) * (3)

Created attachment 362220 [details] Patch

Comment on attachment 362220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362220&action=review r=me > Source/WebInspectorUI/UserInterface/Base/Debouncer.js:79 > + delayForFrame() Up to you but I like these names better: delayForTime() delayUntilNextFrame() delayUntilNextMicrotask() And there could be a convenience: delayUntilNextEventLoop() which is delayForTime(0) > Source/WebInspectorUI/UserInterface/Base/Throttler.js:75 > + if (isNaN(this._lastFireTime)) { > + this._execute(); > + return; > + } If we initialize `this._lastFireTime` to `0` instead of NaN we can eliminate this case entirely and everything should behave as expected. > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:104 > - this[key].addEventListener("input", this.debounce(250)._handleNumberInputInput); > + this[key].addEventListener("input", this._handleNumberInputInput.bind(this)); So this doesn't debounce at all now? Seems fine. > Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:95 > + this._dispatchCurrentRepresentedObjectsDidChange = new Debouncer(() => { Normally you name members with a suffix such as `fooDebouncer` or `fooThrottler`, much like we often do for Maps and Sets. I like that pattern. This one got missed. this._displayCurrentRepresentedObjectsDidChangeDebouncer > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:111 > + this._generateContentThrottler.fire(); Nice, this even eliminates the need to track `index` because when the throttler fires it uses `this._index`! > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:47 > + textEditor.addEventListener(WI.TextEditor.Event.ContentDidChange, (event) => { > + contentDidChangeDebouncer.delayForTime(250, event); > + }, this); This is so much clearer! So much win! > Source/WebInspectorUI/UserInterface/Views/SpringEditor.js:220 > + this._updatePreviewAnimation(event); Another case where we are removing a debounce? Seems fine > LayoutTests/inspector/unit-tests/throttler.html:125 > + throttler.fire(); > + throttler.fire(); > + > + throttler.cancel(); We should assert that this fired once and won't fire the 2nd time. (as you said, just add a log in the throttler)

Comment on attachment 362220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362220&action=review >> Source/WebInspectorUI/UserInterface/Base/Debouncer.js:79 >> + delayForFrame() > > Up to you but I like these names better: > > delayForTime() > delayUntilNextFrame() > delayUntilNextMicrotask() > > And there could be a convenience: > > delayUntilNextEventLoop() which is delayForTime(0) I prefer the alliteration :) >> Source/WebInspectorUI/UserInterface/Base/Throttler.js:75 >> + } > > If we initialize `this._lastFireTime` to `0` instead of NaN we can eliminate this case entirely and everything should behave as expected. Ooooo, I like it!

Created attachment 362636 [details] Patch

Comment on attachment 362636 [details] Patch Clearing flags on attachment: 362636 Committed r242017: <https://trac.webkit.org/changeset/242017>

All reviewed patches have been landed. Closing bug.

<rdar://problem/48349963>