There was one test failure I haven't figured out. I do believe that the profiling results still give an accurate impression of what the speedup would be, though obviously I would need to fix the test failure if it's decided to approve this new approach.

Is there some kind of bug with the V8 profiler Atom is using? I don't understand why it's splitting up the one call to replaceAll into a dozen apparently separate calls in the timeline view.

Hey, nice job on this investigation @jedwards1211! This is a useful starting point.

When I changed replace to use plain JS string operations and then called setTextInBufferRange once to replace the entire buffer text at the end, the same 60000 replacements took about 5.5 seconds

I get the entire text of the buffer, build up the replacement buffer text with native JS string operations, and replace the entire buffer text

I think it's important that we use individual setTextInRange calls for two reasons:

• We need to update cursors, selections, and other markers according to the deltas, rather than losing all of their positions.
• Atom can deal with files that are too large to fit in JavaScript strings. We need to avoid algorithms that require manifesting the entire buffer's text as a String.

The problem is that the BufferSearch's search results are also stored as markers, and there is a cost associated with updating markers when editing the buffer. I think that what replaceAll should do is:

1. Get the ranges of every marker.
2. Destroy the results marker layer, so that it won't need to be updated during subsequent setTextInRange calls.
3. Iterate the ranges from step 1 in reverse, and update the buffer. Iterating the ranges in reverse is important so that as we update the buffer, the subsequent ranges don't become stale.
4. Recompute the marker layer by calling findAndMarkAllInRangeSync, because for a regex search, it's possible that replacing all the matches will create new matches.

☝️ All of this only applies to when replacing all markers though. When replacing just a single marker, we probably want to keep things as they are. Maybe we need to separate those two code paths instead of having them call a common method.

Yeah I figured there are good reasons we wouldn't want to operate on the whole buffer as plain strings.
Do you think anything could be gained from adding the ability to set text in multiple ranges with a single call to text-buffer? Especially considering that there might be many replacements within a given chunk of text (if my understanding of the basic structure of the text buffer is correct)

And any idea why splice is taking so long?

Also

Atom can deal with files that are too large to fit in JavaScript strings. We need to avoid algorithms that require manifesting the entire buffer's text as a String.

That's good...for such a file though, assuming Atom is also capable of handling too many markers to fit in memory, do you think it would make more sense to do the replacements first and rebuild the markers later?

@maxbrunsfeld sorry to bother again but do you have any idea why splice would take so long?

@jedwards1211 splice is expensive because it's called a huge number of times and each time, it's updating a huge number of markers - the markers that represent all the search results. But we don't need to update these markers because we're about to invalidate them all by replacing the text that they're marking. That's why I made the suggestion above.

Oh I see. What about other kinds of markers though -- I'm guessing error underlines are another marker layer that would slow down calls to splice even if the search results markers are destroyed?
And probably git diff plugins use markers as well?

For this reason I'm wondering if we can get even better performance by devising a way to pass multiple changes to the text buffer at once, so that we only have to recompute the markers for each chunk of text once, rather than once for each marker being replaced within that chunk.

Yes those features all use markers as well. It should be fine though; updating markers is already really efficient; in this case there’s just a huge amount of markers that don’t need to be updated.

Well it's a bit absurd to imagine, but if for whatever reason there were 60000 diff markers, it would cause the same performance issue as 60000 search result markers, right?
I'd like to eventually try to make a PR to solve this but I'd prefer to fix the problem once and for all...