applyChange emits lots of events when calling `replace`

[nikhilofthesouth]

More of a question than an issue necessarily.

I noticed when playing around the whitespace's "Convert tabs/spaces to spaces/tabs" functionality that the pane would hang for 3-4 seconds at a time for a decently sized file (say text-buffer.coffee and its 1400 lines of code). After looking at a flame graph and digging into the code, I realized a lot of this time is spent on @emitter.emit calls originating from applyChange being called for each regex replacement (in this case replacing tabs with spaces or vice versa). Specifically, each call to applyChange emits a will-change (which happened to have one listener on my machine), and a did-change listener (which has 4 listeners locally). Disabling the emit events took the run time down to around a third of a second, or about a tenth of the original run time.

Is there a reason why we need to emit so many events in this particular scenario? Would it make sense for calls to replace or even transact to only emit the change events once before and once after all the replacements have concluded? If so, how might that look?

I can post some screenshots to clarify the issue if necessary. Thanks!

[maxbrunsfeld]

@nn2242 You're absolutely right; this is a major problem with the current API for observing changes. Unfortunately, it'll take some time to move to a new API. We can't simply emit fewer change events, because the contract of TextBuffer::onDidChange, which many packages rely upon, is that the events it emits fully describe every change that happens to the buffer.

For backwards-compatibility, we need to introduce a new mechanism for observing changes to the buffer. We're thinking of calling it ::onDidTransact. This method would call its callback at the end of every transaction, and it will need to pass an object that describes all of the changes that occurred in the Transaction. We're thinking of using the recently-introduced Patch object to represent the set of changes.

Once we introduce this API, we'll need to migrate all of our packages to use it instead of ::onDidChange.

We haven't started implementing this, but it is definitely on our roadmap for Atom's performance.