[Bug] debugger; statement in diffAlgorithm.ts

[Prinzhorn]

Reproducible in vscode.dev or in VS Code Desktop?

• [X] Not reproducible in vscode.dev or VS Code Desktop

Reproducible in the monaco editor playground?

• [ ] Not reproducible in the monaco editor playground

Monaco Editor Playground Link

https://microsoft.github.io/monaco-editor/playground.html?source=v0.46.0#XQAAAALqAQAAAAAAAABBqQkHQ5NjdMjwa-jY7SIQ9S7DNlzs5W-mwj0fe1ZCDRFc9ws9XQE0SJE1jc2VKxhaLFIw9vEWSxW3yscw0NV5ZhsAFQjfi1GSTeRDChAtr8RC8jhkfedYn4TxJcJIcii5J3w75DBaWUzZY7JhpoXvJUR_WOC8PUjTJJHARZq1Yvwknih9CKoN8cX52K0IsNBtLsWAS-pNClj345Kw7Obf-TJkCoxNGAe1feBwKEM_zEfs2hqXYosGzEcrts7GBx6xmX5kC3DSKFaUyWT-8ggK3bn1Kgwq8fyVflZBnzqHH7a6CfTk7xzp5I2WObspjo9StazRuvhh6xiLzjVYOH9FGjjODUBwuGK_zD7P_f2Paw

Monaco Editor Playground Code

const originalModel = monaco.editor.createModel(
    'foo bar \n'.repeat(10000),
    "text/plain"
);
const modifiedModel = monaco.editor.createModel(
    'foo foo bar \n'.repeat(10000),
    "text/plain"
);

const diffEditor = monaco.editor.createDiffEditor(
    document.getElementById("container"),
    {
        originalEditable: true,
        automaticLayout: true,
    }
);
diffEditor.setModel({
    original: originalModel,
    modified: modifiedModel,
});

Reproduction Steps

1. Open dev tools
2. Load playground with large diff
3. Will eventually break at debugger statement

Actual (Problematic) Behavior

debugger; :

Expected Behavior

No debugger;

Additional Context

Just upgraded monaco from a pretty old version and now keep getting stuck in the debugger statement while navigating in my app (I work with a lot of large diffs).

Bisected to https://github.com/microsoft/vscode/compare/949c8073e29554a897d764dcd05930bcb626e377...061041479c3fc2ad4d813bc87bc62d2b0c95d0b8

https://github.com/microsoft/vscode/blob/05bf957b312e16532c7669004df043e7c580af1d/src/vs/editor/common/diff/defaultLinesDiffComputer/algorithms/diffAlgorithm.ts#L188

[Prinzhorn]

Added here https://github.com/microsoft/vscode/commit/b31ec2599204624e10b657556acdf100ed6e67c0 by @hediet

The monaco API docs don't give any results for debuggerDisable. I don't know why a debugger; statement would be shipped at all.

Edit: I'm also not sure if this implies that diffing can timeout? I don't want that. I show a spinner, diffing happens in a worker, it can be canceled. There is absolutely no reason for a timeout.

[hediet]

Edit: I'm also not sure if this implies that diffing can timeout?

Exactly. When you debug, the timeout might be hit earlier, or in the worst-case, when you debug the algorithm, you hit it immediately.

However, there is an option and you can set timeout to 0 to disable it completely. Note that there is no way to cancel the diff computation at the moment though.

[Prinzhorn]

However, there is an option and you can set timeout to 0 to disable it completely.

Where exactly (refs https://github.com/microsoft/monaco-editor/issues/4149)?

Note that there is no way to cancel the diff computation at the moment though.

I could've sworn this worked in earlier versions of Monaco. That setModel would abort the currently running diffing (terminate() the worker). But now my PC fans keep blasting...considering this, I definitely do want a timeout just to avoid it running in the background aimlessly (there doesn't seem to be a timeout when dev tools are closed, I kept it running for a few minutes before killing the app). But I also want it to work with large diffs, without needing the timeout to prevent footgunning yourself....

[hediet]

That setModel would abort the currently running diffing (terminate() the worker).

I don't think the worker was ever per model!

But I also want it to work with large diffs, without needing the timeout to prevent footgunning yourself....

In theory, you can implement your own diff algorithm service, which just calls into the existing worker to compute the diff, but with a different timeout object that uses a shared array buffer, so you can signal a termination of the computation from the main thread.

Maybe it also a good idea if our diff algorithm check for incoming termination signals every now and then, but I fear this has a massive impact on performance (the algortihm would have to be async and then probably many v8 optimizations would bail out).

[Prinzhorn]

Sorry for the confusion, it appears even with 0.46.0 this still works as expected and aborts diffing:

originalModel.setValue('some new value');
modifiedModel.setValue('some new value');

Sure, that's not "canceling" in that sense, but exactly what I need and depend on. When you navigate in "diff mode" through a large data set, it won't accumulate diff workers but only ever works on the most recent one.

Maybe it also a good idea if our diff algorithm check for incoming termination signals every now and then, but I fear this has a massive impact on performance (the algortihm would have to be async and then probably many v8 optimizations would bail out).

I know nothing about Monaco's architecture, sounds like you keep using the same worker? I was assuming it launches a new worker every time, because you can trivially terminate() them with an AbortSignal (or similar) without having to worry about cleaning up. That's how I treat a lot of Web Workers and Worker Threads. Just throw work at them and terminate them when they're not needed anymore (e.g. because the user views a different data set now). For user-driven workloads (which Monaco clearly is) this works well, because the cost (and time) of creating new threads is negligible (compared to the user's perception of wall clock time) but the benefits you gain (of not having to clean up or maintaining a pool of workers) is nice. And the worker code does not need to be cooperative and actively listen for signals to stop.