Potential Performance Regression

[firasdib]

Describe the issue

I'm seeing a new bug on regex101 that results in errors sometimes being thrown to the console:

RangeError: Maximum call stack size exceeded.
  at ContentView.replaceChildren(regex101/../node_modules/@codemirror/view/dist/index.js:568:23)
  at replaceRange(regex101/../node_modules/@codemirror/view/dist/index.js:710:16)
  at mergeChildrenInto(regex101/../node_modules/@codemirror/view/dist/index.js:720:5)
  at LineView.merge(regex101/../node_modules/@codemirror/view/dist/index.js:1462:9)
  at replaceRange(regex101/../node_modules/@codemirror/view/dist/index.js:641:16)
  at DocView.updateChildren(regex101/../node_modules/@codemirror/view/dist/index.js:2884:13)
  at DocView.updateInner(regex101/../node_modules/@codemirror/view/dist/index.js:2821:14)
  at DocView.update(regex101/../node_modules/@codemirror/view/dist/index.js:2811:18)
  at EditorView.measure(regex101/../node_modules/@codemirror/view/dist/index.js:7691:44)

This seems to be related to the usage of the "Visualize Whitespace" setting (enabled by default) that the site offers. Without it, performance is snappy.

Here is the code:

import { Decoration, EditorView, MatchDecorator, ViewPlugin, ViewUpdate } from '@codemirror/view';
import { WidgetType } from '@codemirror/view';
import * as styles from './showWhitespace.css';

const spaceDeco = Decoration.mark({
  class: styles.whitespace,
  attributes: { 'data-widget': 'true' },
});
const tabDeco = Decoration.mark({ class: styles.tab, attributes: { 'data-widget': 'true' } });

const decorator = new MatchDecorator({
  regexp: /[ \t]/g,
  decoration: (m) => (m[0] === '\t' ? tabDeco : spaceDeco),
  boundary: /[^ \t]/,
  maxLength: 100,
});

export const whitespacePlugin = ViewPlugin.define(
  (view) => ({
    decorations: decorator.createDeco(view),
    update(u) {
      this.decorations = decorator.updateDeco(u, this.decorations);
    },
  }),
  {
    decorations: (v) => v.decorations,
  }
);

I am aware that this is going to cause some performance issues, since each space/tab is being encapsulated in a tag, but it didn't use to be this slow, nor did the error appear in the console.

Using these versions:

    "@codemirror/commands": "^6.6.2",
    "@codemirror/state": "^6.4.1",
    "@codemirror/view": "^6.33.0",

Browser and platform

Chrome 128

Reproduction link

https://regex101.com/r/iMcJgo/1

[marijnh]

but it didn't use to be this slow, nor did the error appear in the console.

I tested with some other versions of @codemirror/view (going back to 6.17.0) and I'm not seeing a noticeable difference in speed.

I wasn't able to reproduce the stack overflow but I can see how it'll happen (the interface for Array.splice requires all the new elements to be passed as separate arguments, to the library spreads in an array, which will be put onto the call stack, and can be huge, in a situation like this).

But yeah, browser aren't great with huge amounts of inline elements. The time seems mostly spent in layout. To mitigate this, I guess one approach would be to allow MatchDecorator to hold back on creating too many decorations for a single line (though people would of course still be able to create a similar situation though other means). Does that seems like a reasonable direction?

[firasdib]

If there is a way to only create decorators for the elements visible on screen, that would solve the issue. We would still want decorators to be created if the line is being wrapped and fills up the viewport.

If that's not possible, simply limiting the amount of decorators created at once would be better than nothing.

[marijnh]

If there is a way to only create decorators for the elements visible on screen,

This is pretty much what CodeMirror's viewport system is for. But it creates a certain overlap, covering more than the currently visible area, in order to avoid blank space from popping into view during scrolling, and to not have to mess with the dom every time the editor is scrolled a few pixels. The amount content that the editor draws is usually something that browsers can lay out very quickly.

Looking into this some more, it seems that even with the ~11k in-viewport tokens your example produces, I am getting okay performance when I do this locally (and, again, never hit the stack overflow). But on regex101, the performance is a lot worse. So maybe there is some other complicating factor. One thing I did notice is that the way I style these matters—with a :before pseudo-element to show the spaces (which @codemirror/view used before), it's a lot slower than with a background. Inspecting your site, I can see you're already using a background. But maybe some other CSS is giving the browser a hard time.

[firasdib]

Interesting, thanks for the update.

I've debugged this a bit now over my lunch, and I'm consistently getting the stack overflow error in my local environment, but very rarely in production. Not sure what causes that distinction to occur.

I did not know there was a built in extension for this. I will certainly use that instead. I can see that the plugin source is very similar to my code above, both lack if (u.docChanged || u.viewportChanged) { ... } - perhaps that's an optimization to be made instead of updating decorations on every conceivable change?

Is there a way to safeguard from the stackoverflow? Perhaps setting an upper limit to avoid hitting this edge case. Alternatively, split the array up in chunks that can be processed independently.

[marijnh]

perhaps that's an optimization to be made instead of updating decorations on every conceivable change?

MatchDecorator should already take care of that.

I've been thinking about having a custom implementation of Array.splice that allows the content to be passed as a heap array. It'll probably be slower than the built-in method, but at least it won't overflow the stack.

[firasdib]

MatchDecorator should already take care of that.

Understood.

I've been thinking about having a custom implementation of Array.splice that allows the content to be passed as a heap array. It'll probably be slower than the built-in method, but at least it won't overflow the stack.

I think you can get the best of both worlds by checking the size of the array being manipulated, something like:

if (arr.length > TROUBLESOME_SIZE) { safeSplice(...); }
else { regularSplice(...) }

I'd be happy to test it out in my local environment.

[marijnh]

Could you see if attached patch helps for the case where you're seeing these overflows?

[firasdib]

Patch fixes the stack overflow issue. Performance still seems pretty terrible, so much so that my browser locks up for a few seconds when interacting with the document.

I can see from the DOM tree that every single element around a space is being rendered, even if it's well outside of the viewport. Could this issue be related to the fact that it's one long line of text and that line, at least parts of it, is inside the viewport?

[marijnh]

If you compare view.contentDOM.textContent.length to view.state.doc.length you should see a difference. But due to the considerations around scrolling mentioned before, yes, the rendered text will be bigger than the text currently visible on your screen.

[firasdib]

I understand that, but what I'm struggling to understand, do you render all the tokens in the visible lines, or only the tokens visible within the lines?

Because this scenario has a line with 10k+ characters, but we can only see a few percent of those at a time.

[marijnh]

Long lines will be rendered only partially (hence the difference in DOM text length and document length).

[firasdib]

Gotcha, I can see that now. Seems like it renders a lot more than I would have thought. Thank you for your help, I'll have to troubleshoot the performance issues a bit more on my end, as I can't quite figure out why its so slow.

[firasdib]

Any chance you could tag a release with the splice fix?

[marijnh]

Yes—I've tagged 6.34.1