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

[PerfSampleError] #187460

Closed
vscodenpa opened this issue Jul 10, 2023 · 12 comments
Closed

[PerfSampleError] #187460

vscodenpa opened this issue Jul 10, 2023 · 12 comments
Assignees
Labels
error-telemetry VS Code - Issues generated by telemetry terminal Integrated terminal issues under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@vscodenpa
Copy link

[PerfSampleError] by <<renderer>> in createImageBitmap
at createImageBitmap
at createImageBitmap in vscode-file://vscode-app//node_modules.asar/xterm-addon-image/lib/xterm-addon-image.js:2:32040
at _createPlaceHolder in vscode-file://vscode-app//node_modules.asar/xterm-addon-image/lib/xterm-addon-image.js:2:36594
at _open in vscode-file://vscode-app//node_modules.asar/xterm-addon-image/lib/xterm-addon-image.js:2:35610
at s in vscode-file://vscode-app//node_modules.asar/xterm-addon-image/lib/xterm-addon-image.js:2:31218
at activate in vscode-file://vscode-app//node_modules.asar/xterm-addon-image/lib/xterm-addon-image.js:2:50158
at loadAddon in vscode-file://vscode-app//node_modules.asar/xterm/lib/xterm.js:1:247802
at loadAddon in vscode-file://vscode-app//node_modules.asar/xterm/lib/xterm.js:1:277222
at lb in out/vs/workbench/workbench.desktop.main.js:1836:18083

Go to Errors Site

@vscodenpa vscodenpa added the error-telemetry VS Code - Issues generated by telemetry label Jul 10, 2023
@Tyriar
Copy link
Member

Tyriar commented Jul 10, 2023

@jerch FYI I think this means there was a case where this took >10% of a 5 second period (so >500ms) of time that was profiled:

const profile = await nativeHostService.profileRenderer(sessionId, 5000);

if (samples.length === 0 || samples[0].percentage < 10) {
// ignore this profile because 90% of the time is spent inside "special" frames
// like idle, GC, or program
return { kind: ProfilingOutput.Irrelevant, samples: [] };
}
return { kind: ProfilingOutput.Interesting, samples };

I guess the image was just really large? Full screen window + tall terminal + 4k monitor?

I'm seeing this happened 146 time on 127 machines for the latest stable, so over about 6 days.

@Tyriar
Copy link
Member

Tyriar commented Jul 10, 2023

Not sure how actionable this is.

@Tyriar Tyriar added the under-discussion Issue is under discussion for relevance, priority, approach label Jul 10, 2023
@Tyriar Tyriar added this to the Backlog milestone Jul 10, 2023
@Tyriar Tyriar added the terminal Integrated terminal issues label Jul 10, 2023
@jerch
Copy link

jerch commented Jul 10, 2023

I guess the image was just really large? Full screen window + tall terminal + 4k monitor?

Well one of my bigger local test images is 6000x4000px and accounts for ~25MB sixel-encoded - and the overall throughput from term.write to the image finally showing up is ~40MB/s. So a 4k screenshot would get into the +100ms realms for sure.

Still this should not happen during normal image decoding, as all the decoders work chunkwise and return control flow to the sequence parser after one chunk has been processed. Could it be, that the test inserts all the bytes of one big image into one term.write call? Because I dont split chunks further, they just get processed as they flow in assuming that any data provider for term.write already has the data split in chunks by other means (the PTY typically splits data in 4k to 65k byte chunks).

@Tyriar
Copy link
Member

Tyriar commented Jul 11, 2023

Could it be, that the test inserts all the bytes of one big image into one term.write call?

If it was write that wouldn't explain why createImageBitmap is the expensive call though 🤔. I'm going to close this as it's complaining about a browser API call taking too much time and we don't have enough info.

@Tyriar Tyriar closed this as completed Jul 11, 2023
@jerch
Copy link

jerch commented Jul 11, 2023

Ah ok, I did not know that it only means the runtime of that call.

Is the measured time here raw runtime of that call (from call to resolve), or mainthread blocking time? Because the latter should not happen due to being an async call, ofc the first can happen, if the image is quite big.

@Tyriar
Copy link
Member

Tyriar commented Jul 11, 2023

I think I misinterpreted it initially and it means that there was 500ms+ of blocking time (10% of 5 seconds) and the biggest consumer of that (top bottom up profile entry) was createImageBitmap.

@Tyriar
Copy link
Member

Tyriar commented Jul 11, 2023

This is relatively new though and something might be going wrong with the measurement. The async thing in particular confuses me.

@jerch
Copy link

jerch commented Jul 11, 2023

500ms+ of blocking time

Eww, well thats not good. Sadly I dont understand the line numbers the stacktrace gives, so idk which code path ran into that bad timing. From the callback at least it seems to be triggered right from initial addon startup, which makes me wonder, how thats even possible? Wouldn't the terminal (+addon) be spawned much earlier in terms of event loop invocations, before any real data arrives at it?

@Tyriar
Copy link
Member

Tyriar commented Jul 11, 2023

Wouldn't the terminal (+addon) be spawned much earlier in terms of event loop invocations, before any real data arrives at it?

I think you're on to something here. The terminal is started after the "restored" phase which is probably when the perf sample stuff kicks in. There could be an unbounded amount of terminals being restored, across multiple windows (some could be on the same CPU if that matters), each one could have 4 canvases being created. Maybe createImageBitmap, while small, just happens to be the longest call out of all of them?

@jerch
Copy link

jerch commented Jul 11, 2023

Hmm, sounds plausible. It still makes me wonder, even if it had to restore like 20+ terminals at once, why would createImageBitmap that long initially?

Ah well, no clue - I'd say if its not a pressing issue we should track that in the xterm.js repo once the addon got added. Maybe some of my init path stuff creates a high workload, and we are able to defer it somehow.

@jerch
Copy link

jerch commented Jul 11, 2023

Might have found the issue taking pretty long during startup - seems thats related to the on-the-fly placeholder generation, which currently happens during addon init. Should be able to defer that as well.

@jerch
Copy link

jerch commented Jul 11, 2023

jerch/xterm-addon-image#56 should remove most of the addon init burden, its lit. down to a few variable assigments. Everything else got deferred.

Note that this wont help with blocking time, if there is a rather big single term.write(really_big_image_data) call going on somewhere. If thats an issue, then we might have to look at the incoming write buffer logic of xtermjs, and whether there should be a split logic for overly big chunks (cannot really orchestrate that from deep within parser handlers).

@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
error-telemetry VS Code - Issues generated by telemetry terminal Integrated terminal issues under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants