Speed up generation of output #564

AlCalzone · 2023-03-19T22:52:20Z

This is a proof of concept for a different rendering approach that does not rely on sliceAnsi as heavily.

I tried this with my test component from #560, but with 50 spinners. Without this change, 50 spinners put one core of my CPU at 100%, and rendering lags heavily. With this change, I'm looking at ~40% utilization of one core, which is still not ideal, but renders fluently.

The output for 10s of profiling now looks like this, is idle half the time and most of the work is done in wasm, which is probably from yoga internals:

fixes: #560

This will need some more testing with emojis and clipping, but I wanted to get this out for some early feedback.

AlCalzone · 2023-03-20T18:38:40Z

Ok, looks like emojis and some full-width characters are causing test failures, I'll look into that.

And there's probably an ANSI code in a different position than before - I don't see a visual difference here:

vadimdemedes · 2023-04-02T17:42:39Z

This is really impressive! Would you be willing to push this PR to completion?

AlCalzone · 2023-04-02T18:55:00Z

Yeah, will do soon.

AlCalzone · 2023-04-05T21:23:57Z

Tests are passing too now. Just for fun, I ran time npm run benchmark benchmark/simple to compare.
master

real    0m11,412s
user    0m13,342s
sys     0m0,604s

this PR

real    0m8,208s
user    0m9,051s
sys     0m0,330s

So roughly 32% faster rendering for even simple outputs.

vadimdemedes · 2023-04-08T08:32:27Z

src/output.ts

+						// Some characters take up more than one column. In that case, the following
+						// pixels need to be cleared to avoid printing extra characters
+						const charWidth = char.fullWidth ? 2 : char.value.length;
+						for (let dx = 1; dx < charWidth; dx++) {


Why does this skip the first character and starts from the second one?

Because the first char is the char that's actually written, so width 1 is already accounted for.

Not sure I understand, could you elaborate the "that's actually written" part?

Line 201 writes the current character to the output array. Some characters are wider than a single column though, so we have to set the following columns (up to the character width) to "" so we don't end up with extra characters.
So if the char is a 2-column emoji, we write the emoji to column x and "" to column x+1. The character after the emoji will then correctly be at column x+2.

Do we need a loop here then? Could it be simplified to this?

if (char.fullWidth) { currentLine[offsetX + 1] = /* empty char */ } offsetX += character.fullWidth ? 2 : 1

Aren't there some emojis which have length > 4?

Nevermind, they have a visual width of 2. We can't fully go by the "fullWidth" property though, since some emojis with a visual width of 2 are not detected by https://github.com/sindresorhus/is-fullwidth-code-point
For example 🍦 (code point 0x1f366) and 🎉 (code point 0x1f389)

I've pushed a commit to eliminate the loop.

vadimdemedes · 2023-04-08T08:32:43Z

Superb work 🔥

vadimdemedes · 2023-04-24T06:33:32Z

src/output.ts

@@ -198,12 +198,16 @@ export default class Output {
 					const chars = styledCharsFromTokens(tokenize(line));
 					let offsetX = x;
 					for (const char of chars) {
+						if (offsetX >= this.width) break;


How did you run into this issue where an out-of-bounds write would occur? Could you add a test for it to ensure it doesn't regress back?

Done. In my case it was a pretty complicated nested layout, but I managed to reduce it to a <Text wrap="end"> that's longer than the stdout width.

There's no wrap="end" value, but there's wrap="truncate-end". See https://github.com/vadimdemedes/ink#wrap. I updated it in the test you added, then commented out "prevent out-of-bounds writes" commit changes and it didn't crash for me. Looks like we don't need this code then?

Hmm maybe the test case isn't correct then. I ended up with gaps in the output array, which crashed the function that converts tokens back to a string.

Do you still see the same issue when using wrap="truncate-end" instead of wrap="end" (which is an invalid prop)?

Will try tomorrow or so.

Hey @AlCalzone, do you still have the capacity to finish this PR? Would really love to ship it. No worries if you're busy though.

Sorry for the delay. I'll try to look at this soon. The problem is I'm actually not sure how to trigger the out-of-bounds write.
It did happen for me in a relatively complicated flexbox layout, which is why I added the checks.

No worries, take your time 👍

AlCalzone · 2023-07-06T19:07:00Z

@vadimdemedes I finally managed to reproduce this reliably. The issue is when trying to render boxes with borders that are at least 2 columns too wide for the terminal, e.g. this component on a terminal with width 10:

<Box width={12} height={10} borderStyle={'round'} flexDirection="row"></Box>

Not sure why this doesn't happen for width+1 though.

This case causes the following writes to output:

Write "╭──────────╮" at x=0, y=0, w=10
Write "│" at x=0, y=1, w=10
Write "│" at x=0, y=2, w=10
Write "│" at x=0, y=3, w=10
Write "│" at x=0, y=4, w=10
Write "│" at x=0, y=5, w=10
Write "│" at x=0, y=6, w=10
Write "│" at x=0, y=7, w=10
Write "│" at x=0, y=8, w=10
Write "" at x=0, y=9, w=10
Write "│" at x=11, y=1, w=10
Write "│" at x=11, y=2, w=10
Write "│" at x=11, y=3, w=10
Write "│" at x=11, y=4, w=10
Write "│" at x=11, y=5, w=10
Write "│" at x=11, y=6, w=10
Write "│" at x=11, y=7, w=10
Write "│" at x=11, y=8, w=10
Write "" at x=11, y=9, w=10
Write "╰──────────╯" at x=0, y=9, w=10

As a result, the arrays for lines 1...9 have an entry at index 11 that should not exist and index 10 is empty, essentially this:

This causes styledCharsToString to throw because it doesn't expect undefined entries in the input. By simply not setting the out-of-bounds items in the output array, this doesn't happen.

I added a better test case for this.

vadimdemedes · 2023-07-16T08:30:06Z

Thanks @AlCalzone, that was really helpful! Here's what I've found.

I took your Box example and rendered it in the terminal.

import React from 'react';
import {Box, render} from 'ink';

function Test() {
	return <Box width={12} height={10} borderStyle={'round'}></Box>;
}

process.stdout.columns = 10;
render(<Test />, {debug: true});

The right border is rendered in the wrong place, even though there are correct X and Y coordinates for all sides:

Top border has X = 0, Y = 0
Left border has X = 0, Y = 1
Right border has X = 11, Y = 1
Bottom border has X = 0, Y = 9

Here's where each line is written to output:

ink/src/output.ts

Lines 187 to 190 in 8a04760

    
           output[y + offsetY] = 
        
           	sliceAnsi(currentLine, 0, x) + 
        
           	line + 
        
           	sliceAnsi(currentLine, x + width);

I added this console.log to see what exactly gets written and where:

console.log({
	// X coordinate of the `line` to write to output
	x,
	// Existing output before X coordinate
	before: sliceAnsi(currentLine, 0, x),
	beforeWidth: sliceAnsi(currentLine, 0, x).length,
	// New text to write to output
	line,
	lineWidth: line.length,
	// Existing output after X coordinate + width of the `line`
	after: sliceAnsi(currentLine, x + width),
	afterWidth: sliceAnsi(currentLine, x + width).length
});

Here's the output for one of the lines for writing the right border:

{
  x: 11,
  before: '│         ',
  beforeWidth: 10,
  line: '│',
  lineWidth: 1,
  after: '',
  afterWidth: 0
}

Normally, sliceAnsi(currentLine, 0, x) should return a string where length === x, but here beforeWidth equals 10, not 11. The reason is currentLine comes from the output variable, which is initialized with strings that are as wide as the terminal (in our case terminal width is 10).

ink/src/output.ts

Line 174 in 8a04760

const currentLine = output[y + offsetY];

ink/src/output.ts

Lines 97 to 99 in 8a04760

    
           for (let y = 0; y < this.height; y++) { 
        
           	output.push(' '.repeat(this.width)); 
        
           }

This creates a problem where output contains lines with the length of 10, so the index of the last character in each line is 9. At the same time, we're trying to write a character at index 11, which results in invalid output where character at index 10 is missing. As a result, right border is placed at X = 10, not X = 11.

It doesn't cause a crash in the current version of Ink, because sliceAnsi is used to extract string ranges, which doesn't fail if you run sliceAnsi(str, 0, 11) where str.length === 10. However, your PR no longer uses sliceAnsi and is sensitive to undefined items.

To sum up, this is an existing issue of Ink with content larger than the terminal and should be fixed separately from this PR. To avoid regressions in this PR though, we need to skip undefined array items to keep the rendering behavior the same as it is now.

vadimdemedes · 2023-07-16T08:41:32Z

Pushed a commit to keep existing behavior with out of bounds writes.

vadimdemedes · 2023-07-16T09:17:50Z

CI is green, merging!

Thank you for this incredible performance boost, @AlCalzone!

AlCalzone added 4 commits April 5, 2023 22:42

perf: optimize how ansi-codes are handled in Output.get()

6e89620

make eslint happy

d723343

use published package

39f5e27

fix failing tests

0eb14cd

AlCalzone force-pushed the pixels branch from 235718e to 0eb14cd Compare April 5, 2023 21:10

AlCalzone marked this pull request as ready for review April 5, 2023 21:12

fix lint

41b7eb3

vadimdemedes changed the title ~~perf: optimize how ansi-codes are handled in Output.get()~~ Speed up generation of output Apr 8, 2023

vadimdemedes reviewed Apr 8, 2023

View reviewed changes

fix: don't use loop

25e8782

AlCalzone requested a review from vadimdemedes April 17, 2023 10:23

fix: prevent out-of-bounds writes

4db0600

AlCalzone mentioned this pull request Apr 17, 2023

Implement subtree rendering #21

Open

vadimdemedes reviewed Apr 24, 2023

View reviewed changes

AlCalzone requested a review from vadimdemedes April 26, 2023 13:29

add test for out-of-bounds write

7d6c405

AlCalzone force-pushed the pixels branch from fac2b38 to 7d6c405 Compare July 6, 2023 19:06

Keep existing behavior with out of bounds writes

56b8448

vadimdemedes added 4 commits July 16, 2023 11:44

Minor stylistic changes

b7ddb68

Merge branch 'master' into alcalzone-pixels

147b264

Fix lint errors

d70a2fa

More minor stuff

eb3b8ae

vadimdemedes merged commit 9ff4d20 into vadimdemedes:master Jul 16, 2023
3 checks passed

matteodepalo mentioned this pull request Sep 7, 2023

Fix broken link escapes #623

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up generation of output #564

Speed up generation of output #564

AlCalzone commented Mar 19, 2023 •

edited

AlCalzone commented Mar 20, 2023

vadimdemedes commented Apr 2, 2023

AlCalzone commented Apr 2, 2023

AlCalzone commented Apr 5, 2023

vadimdemedes Apr 8, 2023

AlCalzone Apr 8, 2023

vadimdemedes Apr 8, 2023

AlCalzone Apr 8, 2023

vadimdemedes Apr 10, 2023 •

edited

AlCalzone Apr 10, 2023

AlCalzone Apr 11, 2023 •

edited

AlCalzone Apr 11, 2023

vadimdemedes commented Apr 8, 2023

vadimdemedes Apr 24, 2023

AlCalzone Apr 26, 2023

vadimdemedes Apr 28, 2023 •

edited

AlCalzone Apr 28, 2023 •

edited

vadimdemedes Apr 28, 2023

AlCalzone Apr 28, 2023

vadimdemedes Jun 5, 2023

AlCalzone Jun 7, 2023

vadimdemedes Jun 11, 2023

AlCalzone commented Jul 6, 2023

vadimdemedes commented Jul 16, 2023

vadimdemedes commented Jul 16, 2023

vadimdemedes commented Jul 16, 2023

Speed up generation of output #564

Speed up generation of output #564

Conversation

AlCalzone commented Mar 19, 2023 • edited

AlCalzone commented Mar 20, 2023

vadimdemedes commented Apr 2, 2023

AlCalzone commented Apr 2, 2023

AlCalzone commented Apr 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes Apr 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlCalzone Apr 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Apr 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes Apr 28, 2023 • edited

Choose a reason for hiding this comment

AlCalzone Apr 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlCalzone commented Jul 6, 2023

vadimdemedes commented Jul 16, 2023

vadimdemedes commented Jul 16, 2023

vadimdemedes commented Jul 16, 2023

AlCalzone commented Mar 19, 2023 •

edited

vadimdemedes Apr 10, 2023 •

edited

AlCalzone Apr 11, 2023 •

edited

vadimdemedes Apr 28, 2023 •

edited

AlCalzone Apr 28, 2023 •

edited