Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve repl output window performance #2011

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ Changes to Calva.

## [Unreleased]

- Fix: [Rogue loops that print output to stdout cause Calva to hang](https://github.com/BetterThanTomorrow/calva/issues/2010)
- Fix: [Output window becomes very slow when number of lines of content is very high](https://github.com/BetterThanTomorrow/calva/issues/804)
- Partial Fix: [REPL is Slow and Performance Degrades as the Output Grows](https://github.com/BetterThanTomorrow/calva/issues/942)

## [2.0.323] - 2023-01-07

- Fix: [Provider completions not handling errors gracefully](https://github.com/BetterThanTomorrow/calva/issues/2006)
Expand Down
10 changes: 10 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,16 @@
"lsp"
]
},
"calva.replOutputThrottleRate": {
"markdownDescription": "If the repl outputs too quickly then results will be dropped from the output window. Setting this to 0 will disable throttling.",
"type": "number",
"default": 100
},
"calva.replOutputMaxLines": {
"markdownDescription": "The maximum number of lines to retain in the repl output window. Having the repl output window grow too large can significantly affect performance. Setting this to 0 will disable truncating",
"type": "number",
"default": 1000
},
Comment on lines +778 to +782
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's generally not the number of lines that is the problem, rather the number of tokens on the top level, or in a sexpr. A way to count tokens on the top level is to count words when printing output. That will introduce state that can be tricky to manage, though...

1000 lines is very low. It could be some 50 evaluations of not too big structures. This would be completely non-problematic for Calva to handle. While printing 1000 lines of output with 100 words each might start to be problematic (I actually don't know where the problems start.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's generally not the number of lines that is the problem, rather the number of tokens on the top level, or in a sexpr

While this makes sense, I don't think the additional complexity needed to implement this is really worth it. My assumption is that counting lines is a close-enough analogy in the average case for this to probably be sufficient. Especially if we make sure the threshold is high enough for the user to not care about the truncation.

1000 lines is very low. It could be some 50 evaluations of not too big structures

This value was chosen pretty arbitrary, I'd be happy to increase it to a degree. While working on this I started to see noticeable performance degradation after about 10k lines of stack-trace content (I was printing Exceptions to stdout) so I don't think it should be increased too much. Maybe 5k lines?

I also think it's unlikely that a user would really be scrolling that far back in the repl history too often for us to need to worry about truncating after a few thousand lines.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlikely, maybe, but I know of a user who does this all the time. 😄 I search the file rather than scroll, but anyway. (I keep tons of browser tabs open as well.) To me it makes sense to disable this limit feature (by setting limit to 0, e.g.) and to have that as default.

Copy link
Member

@bpringe bpringe Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @julienvincent, thanks for taking time to look into this.

I think deleting at an arbitrary line length will cause unbalanced forms and break syntax highlighting and maybe also formatting, unless I'm missing something. If I'm not though, I don't think that's really an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it makes sense to disable this limit feature (by setting limit to 0, e.g.) and to have that as default.

I'm happy with that too.

I think deleting at an arbitrary line length will cause unbalanced forms and break syntax highlighting ...

It does break highlighting but in a localised way (only the broken form will be colored incorrectly). The rest of the file maintains the correct highlighting. I think this is an probably an acceptable behaviour.

and maybe also formatting

Which formatting do you mean? I haven't noticed anything obviously wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Here can see what I mean

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way the structural editor works, when there is broken balance in the document, all bets are off. It will sometimes seem to work (or even work), but in many cases the structural editor is broken and with that a lot of Calva functionality that depends on this.

If we limit on lines, it will have to do it such that the balance is never broken. There are some ways to do this, I can think of two right now:

  1. Add the missing brackets. There is code for that in Calva, even if it might be a bit tricky to re-use (or, messy code, tbh).
  2. Always print full results. The limit then need to happen before the buffering, I think. (Not that I know how it is done now, but anyway.)

That said, I think we could also consider popping up a warning when the line limit is reached, instead of enforcing the limit. The warning can have a button for clearing the window.

Copy link
Member

@bpringe bpringe Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does break highlighting but in a localised way (only the broken form will be colored incorrectly). The rest of the file maintains the correct highlighting. I think this is an probably an acceptable behaviour.

Yeah, I'd say that, in particular, is acceptable too.

By formatting I meant if someone is typing code in the repl window, the auto-formatting could/would be broken if there's an unbalanced form (which Peter mentioned above - structural editing). And as he said other functionality may be broken too in that case.

"calva.depsEdnJackInExecutable": {
"markdownDescription": "Which executable should Calva Jack-in use for starting a deps.edn project? The default is to let Calva choose. It will choose `clojure` if that is installed and working. Otherwise `deps.clj`, which is bundled with Calva, will be used. (This settings has no effect on Windows, where `deps.clj` will always be used.)",
"enum": [
Expand Down
5 changes: 5 additions & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ const REPL_FILE_EXT = 'calva-repl';
const KEYBINDINGS_ENABLED_CONFIG_KEY = 'calva.keybindingsEnabled';
const KEYBINDINGS_ENABLED_CONTEXT_KEY = 'calva:keybindingsEnabled';

const REPL_OUTPUT_THROTTLE_RATE_CONFIG_KEY = 'calva.replOutputThrottleRate';
const REPL_OUTPUT_MAX_LINES_CONFIG_KEY = 'calva.replOutputMaxLines';

type ReplSessionType = 'clj' | 'cljs';

// include the 'file' and 'untitled' to the
Expand Down Expand Up @@ -234,6 +237,8 @@ export {
REPL_FILE_EXT,
KEYBINDINGS_ENABLED_CONFIG_KEY,
KEYBINDINGS_ENABLED_CONTEXT_KEY,
REPL_OUTPUT_THROTTLE_RATE_CONFIG_KEY,
REPL_OUTPUT_MAX_LINES_CONFIG_KEY,
documentSelector,
ReplSessionType,
getConfig,
Expand Down
53 changes: 53 additions & 0 deletions src/results-output/results-doc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ import { formatAsLineComments, splitEditQueueForTextBatching } from './util';

const RESULTS_DOC_NAME = `output.${config.REPL_FILE_EXT}`;

const REPL_OUTPUT_THROTTLE_RATE = vscode.workspace
.getConfiguration()
.get<number>(config.REPL_OUTPUT_THROTTLE_RATE_CONFIG_KEY);
const REPL_OUTPUT_MAX_LINES = vscode.workspace
.getConfiguration()
.get<number>(config.REPL_OUTPUT_MAX_LINES_CONFIG_KEY);

const PROMPT_HINT = 'Use `alt+enter` to evaluate';

const START_GREETINGS = [
Expand Down Expand Up @@ -303,10 +310,22 @@ async function writeToResultsDoc({ text, onAppended }: ResultsBufferEntry): Prom
const insertPosition = doc.positionAt(Infinity);
const edit = new vscode.WorkspaceEdit();
const editText = util.stripAnsi(text);

if (REPL_OUTPUT_MAX_LINES > 0 && doc.lineCount > REPL_OUTPUT_MAX_LINES) {
edit.delete(
docUri,
new vscode.Range(
new vscode.Position(0, 0),
new vscode.Position(doc.lineCount - REPL_OUTPUT_MAX_LINES, 0)
)
);
}

edit.insert(docUri, insertPosition, editText);
if (!((await vscode.workspace.applyEdit(edit)) && (await doc.save()))) {
return;
}

onAppended?.(
new vscode.Location(docUri, insertPosition),
new vscode.Location(docUri, doc.positionAt(Infinity))
Expand All @@ -331,6 +350,16 @@ export interface OnAppendedCallback {

let resultsBuffer: ResultsBuffer = [];

type BufferThrottleState = {
count: number;
dropped: number;
timeout?: NodeJS.Timeout;
};
const throttleState: BufferThrottleState = {
count: 0,
dropped: 0,
};

async function writeNextOutputBatch() {
if (!resultsBuffer[0]) {
return;
Expand Down Expand Up @@ -366,6 +395,30 @@ async function flushOutput() {

/* If something must be done after a particular edit, use the onAppended callback. */
export function append(text: string, onAppended?: OnAppendedCallback): void {
if (REPL_OUTPUT_THROTTLE_RATE > 0) {
throttleState.count++;

if (!throttleState.timeout) {
throttleState.timeout = setTimeout(() => {
if (throttleState.dropped > 0) {
resultsBuffer.push({
text: `;; Dropped ${throttleState.dropped} items from output due to throttling\n`,
});
void flushOutput();
}

throttleState.timeout = undefined;
throttleState.count = 0;
throttleState.dropped = 0;
}, 500);
}

if (throttleState.count > REPL_OUTPUT_THROTTLE_RATE) {
throttleState.dropped++;
return;
}
}

resultsBuffer.push({ text, onAppended });
void flushOutput();
}
Expand Down
2 changes: 1 addition & 1 deletion src/results-output/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function splitEditQueueForTextBatching(
const nextBatch = takeWhile(editQueue, (value, index) => {
return index < maxBatchSize && !value.onAppended;
}).map((x) => x.text);
const remainingEditQueue = [...editQueue].slice(nextBatch.length);
const remainingEditQueue = editQueue.slice(nextBatch.length);
return [nextBatch, remainingEditQueue];
}

Expand Down