Skip to content

Comments

improve performance (+99.5% throughput)#37

Closed
AlCalzone wants to merge 5 commits intochalk:mainfrom
AlCalzone:perf
Closed

improve performance (+99.5% throughput)#37
AlCalzone wants to merge 5 commits intochalk:mainfrom
AlCalzone:perf

Conversation

@AlCalzone
Copy link
Contributor

@AlCalzone AlCalzone commented Mar 16, 2023

As mentioned in vadimdemedes/ink#560, I'm having severe performance issues with this library. This PR is an attempt at making it faster.

I suggest reviewing the changes commit-by-commit, while reading the corresponding change description and benchmark. The last commit is a complete rewrite though.

I'm testing the impact of each change using the following benchmark:

import b from "benny";
import sliceAnsi from "./original.js";
// import optimized variants

// This might seem ridiculous, but it's from the test case I posted over at the `ink` issue.
const line = "\x1B[48;2;255;255;255m│\x1B[49m\x1B[37m╚═╝\x1B[39m\x1B[37m╚═╝\x1B[39m\x1B[37m╚═╝\x1B[39m\x1B[37m╚═╝\x1B[39m\x1B[37m╚═╝\x1B[39m\x1B[37m╚═╝\x1B[39m\x1B[37m╚═╝\x1B[39m\x1B[37m╚═╝\x1B[39m\x1B[37m╚═╝\x1B[39m\x1B[37m╚═╝\x1B[39m                                                                                                                                                                             \x1B[34m│\x1B[39m";

const add = (name, fn) =>
	b.add(name, fn, {
		initCount: 10000,
	});

b.suite(
	"sliceAnsi",
	add("original", () => {
		sliceAnsi(line, 0, 82);
		sliceAnsi(line, 82, 83);
		sliceAnsi(line, 83);
	}),
	// test optimized variants

	b.cycle(),
	b.complete()
);

The results are varying between tests a bit, but I'll try to post comparable benchmarks for each change.

First change: Use a simple for loop instead of calling entries() on the character array

Result:

  original:
    21 976 ops/s, ±6.14%   | slowest, 10.01% slower

  for loop:
    24 420 ops/s, ±2.74%   | fastest

@AlCalzone
Copy link
Contributor Author

Second change: Use a string search instead of RegExp to match ansi codes

Result:

  original:
    23 692 ops/s, ±0.73%   | slowest, 17.61% slower

  for loop:
    27 710 ops/s, ±0.94%   | 3.64% slower

  for-loop + no regex:
    28 756 ops/s, ±0.58%   | fastest

@AlCalzone
Copy link
Contributor Author

Third change: Don't spread the string into an array, loop through codepoints directly

Result:

  original:
    24 256 ops/s, ±2.36%   | slowest, 41.81% slower

  for loop:
    28 719 ops/s, ±2.07%   | 31.1% slower

  for-loop + no regex:
    30 076 ops/s, ±1.17%   | 27.85% slower

  no regex + no spread:
    41 685 ops/s, ±0.89%   | fastest

@AlCalzone AlCalzone changed the title improve performance (WIP) improve performance (WIP, currently +40% throughput) Mar 16, 2023
@AlCalzone AlCalzone changed the title improve performance (WIP, currently +40% throughput) improve performance (WIP, currently +70% throughput) Mar 16, 2023
@AlCalzone
Copy link
Contributor Author

4th change: Avoid converting ansi codes to number for end code lookup

Result:

  original:
    27 402 ops/s, ±0.76%   | slowest, 41.57% slower
    
  (snip)

  no regex + no spread:
    43 911 ops/s, ±0.81%   | 6.36% slower

  no conversion between number and string:
    46 895 ops/s, ±0.98%   | fastest

@AlCalzone
Copy link
Contributor Author

5th change: Don't use string.split to access first char before first ";", use indexOf and slice

Result:

  original:
    22 828 ops/s, ±1.84%   | slowest, 49.89% slower

  (snip)

  no conversion between number and string:
    42 327 ops/s, ±0.90%   | 7.08% slower

  indexOf instead of splitting:
    45 553 ops/s, ±0.90%   | fastest

@AlCalzone AlCalzone changed the title improve performance (WIP, currently +70% throughput) improve performance (WIP, currently +99.5% throughput) Mar 16, 2023
@AlCalzone

This comment was marked as outdated.

@AlCalzone AlCalzone changed the title improve performance (WIP, currently +99.5% throughput) improve performance (+149% throughput, +300% possible with memoization) Mar 18, 2023
@AlCalzone AlCalzone marked this pull request as ready for review March 18, 2023 23:37
@AlCalzone AlCalzone changed the title improve performance (+149% throughput, +300% possible with memoization) improve performance (+149% throughput, +300% possible on repeated slices) Mar 18, 2023
test.js Outdated
t.is(sliceAnsi(output, 0, 7), `${chalk.black.bgYellow(' RUNS ')} `);
t.is(sliceAnsi(output, 0, 8), `${chalk.black.bgYellow(' RUNS ')} `);
t.is(JSON.stringify(sliceAnsi('\u001B[31m' + output, 0, 4)), JSON.stringify(`\u001B[31m${chalk.black.bgYellow(' RUN')}`));
t.is(JSON.stringify(sliceAnsi('\u001B[31m' + output, 0, 4)), JSON.stringify(chalk.black.bgYellow(' RUN')));

This comment was marked as outdated.

test.js Outdated

test('support true color escape sequences', t => {
t.is(sliceAnsi('\u001B[1m\u001B[48;2;255;255;255m\u001B[38;2;255;0;0municorn\u001B[39m\u001B[49m\u001B[22m', 0, 3), '\u001B[1m\u001B[48;2;255;255;255m\u001B[38;2;255;0;0muni\u001B[22m\u001B[49m\u001B[39m');
t.is(sliceAnsi('\u001B[1m\u001B[48;2;255;255;255m\u001B[38;2;255;0;0municorn\u001B[39m\u001B[49m\u001B[22m', 0, 3), '\u001B[1m\u001B[48;2;255;255;255m\u001B[38;2;255;0;0muni\u001B[39m\u001B[49m\u001B[22m');

This comment was marked as outdated.

@vadimdemedes
Copy link

I think it'd be a lot easier for maintainers to review this PR and much more likely to merge your changes, if it was split up into much smaller PRs, focused on making each individual performance optimization. Right now it's hard to understand, because the whole library basically get rewritten, which would be a tough sell.

@AlCalzone
Copy link
Contributor Author

AlCalzone commented Mar 20, 2023

Guess I can rip out the last two commits and put them up as an alternative. The others consecutively build on each other and sometimes undo what a previous one did, so splitting them into individual PRs doesn't make much sense to me.

Edit: done

@AlCalzone AlCalzone changed the title improve performance (+149% throughput, +300% possible on repeated slices) improve performance (+99.5% throughput) Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants