Atom-ide-ui: Optimize debounced editor events

[hansonw]

We use RxJS to listen to (and debounce) various editor events. The hottest events are those that occur on every keystroke, namely:

• buffer.onDidChange()
• buffer.onDidChangeText()
• editor.onDidChangeCursorPosition() / cursor.onDidChangePosition

Unfortunately it seems like RxJS debounceTime implementation (which uses its AsyncScheduler by default) seems to be relatively inefficient, taking up to a millisecond for each of the above events.

Flame graph from @nathansobo of a keypress event:

Zip of @nathansobo's timeline profile: typing.zip

I put together a small benchmark where I create an observable and compare the cost of using debounceTime versus just using lodash.debounce (hopefully it's at least somewhat realistic):

https://gist.github.com/hansonw/f7b7f4b32011ee09916dc27a45a43845

With that in mind, it may make sense to enforce that we use observableFromSubscribeFunction in combination with a more standard debounce function rather than using debounceTime throughout the Atom IDE / Nuclide codebases.

[hansonw]

My proposal might be debouncedObservableFromSubscribeFunction:

function debouncedObservableFromSubscribeFunction<T>(
  fn: SubscribeFunction<T>,
  debounceTime: number,
): Observable<T> {
  return observableFromSubscribeFunction(cb => {
    const debounced = debounce(cb, debounceTime);
    const disposable = fn(debounced);
    return new UniversalDisposable(disposable, debounced);
  });
}

[nathansobo]

• buffer.onDidChange()
• buffer.onDidChangeText()
• editor.onDidChangeCursorPosition() / cursor.onDidChangePosition

Do you need to listen to both onDidChangeText and onDidChange? The former should summarize the latter. You really shouldn't listen to onDidChange at all if you can help it.

Another question, not sure about the nature of the code that is running here... do you actually need to debounce this event, or are you just worried about redundant calls in a given tick of the event loop? If so, might it be good enough to run the code in a nextTick or set a single timer in the nextTick.

Assuming you really need the full debounce behavior...

The results actually match what's shown in the profile very well, with the next() calls with debounceTime taking about ~1ms each while the more standard debounce takes about 0.16ms.

I wonder if lodash.debounce is maximally fast. Probably. 0.16ms feels about right just for the overhead of clearing and setting a timeout, which is unfortunately still pretty slow.

I really think it's worth a lot of effort to squeeze out every microsecond on the typing code path. With that in mind, let me throw an idea out there that may go too far. I have not coded this or benchmarked it or anything, so take it with a heap of salt...

Since repeatedly clearing and setting timeouts seems to be expensive, I wonder if a "good enough" debounce could be implemented with a single setInterval/clearInterval pair. The first call would set the interval with some fraction of the desired debounce delay. Subsequent calls would just mutate a timestamp. When our interval function runs, we only run the debounced function if we're within a window of our desired delay. It wouldn't be perfectly precise, but it would avoid the overhead of clearing and setting new timeouts on each call.

[matthewwithanm]

I just did a quick test with this and the results are pretty comparable to the plain lodash one:

const debounce2 = (x, delay) => {
  return Observable.create(observer => {
    let canceled = false;
    const debounced = debounce(v => {
      if (!canceled) observer.next(v);
    }, delay);
    const sub = x.subscribe(debounced);
    sub.add(() => {
      canceled = true;
    });
    return sub;
  });
};

[hansonw]

After converting everything to use a fastDebounce operator as @matthewwithanm described above, this looks much better:

It's still about 0.5ms for the timer setup, but subsequent keystrokes within the debounce window are free! I don't know if I can feel the difference but this is a really easy change to make so we should probably do it.

This was particularly noticeable because we had one source wired up to debounce both text changes and cursor changes (for code highlights)!

[nathansobo]

Great to see. It definitely looks better. ✨

Can I ask again why you're handling onDidChange in addition to onDidChangeText? You'd have much less impact on multi-cursor performance if you only used onDidChangeText, as that event is batched.

[hansonw]

Ah yeah, I've been meaning to migrate our callsites over to using that one
:)

On Tue, Oct 17, 2017 at 9:53 PM Nathan Sobo [e-mail]
wrote:

Great to see. It definitely looks better. ✨

Can I ask again why you're handling onDidChange in addition to
onDidChangeText? You'd have much less impact on multi-cursor
performance if you only used onDidChangeText, as that event is batched.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/facebook-atom/atom-ide-ui/issues/93#issuecomment-337460064,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAjPYlh-HA2EXvKbLjhk9_WUrOCZ9HWcks5stYRCgaJpZM4P9GaK
.

[hansonw]

Fix is in v0.5.2.

[nathansobo]

💥