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 performance (+99.5% throughput) #37

Closed
wants to merge 5 commits into from

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(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
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.

None yet

2 participants