faster wcwidth with lookup table #798

jerch · 2017-07-16T11:36:12Z

This PR optimizes wcwidth by using a lookup table for BMP characters. For a BMP char it boils down to a simple table lookup with some shifting magic to pack the data into 16k bytes. Speedup is at least 10 times. Equality tests included.

coveralls · 2017-07-16T11:40:13Z

Coverage decreased (-0.1%) to 70.039% when pulling b965d0a on jerch:faster_wcwidth into 48dab49 on sourcelair:master.

coveralls · 2017-07-16T11:47:56Z

Coverage decreased (-0.1%) to 70.039% when pulling dd0fcb6 on jerch:faster_wcwidth into 48dab49 on sourcelair:master.

coveralls · 2017-07-16T11:53:05Z

Coverage increased (+0.4%) to 70.581% when pulling 1722248 on jerch:faster_wcwidth into 48dab49 on sourcelair:master.

coveralls · 2017-07-16T12:09:40Z

Coverage increased (+0.5%) to 70.601% when pulling 6733c6c on jerch:faster_wcwidth into 48dab49 on sourcelair:master.

coveralls · 2017-07-16T12:13:13Z

Coverage increased (+0.5%) to 70.601% when pulling 5d87b54 on jerch:faster_wcwidth into 48dab49 on sourcelair:master.

Tyriar

Nice ☺️

Tyriar · 2017-07-16T16:54:14Z

src/InputHandler.ts

+    }
+    return function (num) {
+        num = num | 0;
+        return (num < 65536) ? (table[num >> 2] >> ((num & 3) * 2)) & 3 : wcwidthHigh(num);


Should this have an early return for ascii characters as they are the vast majority, or would that not boost perf much at this point?

Could do with a comment explaining the bit shifting part

@Tyriar Well it gives 5% speedup to exclude the ascii range from the shifting & lookup. So yeah gonna change it.

Tyriar · 2017-07-16T16:54:59Z

src/InputHandler.ts

+        return 1;
+    }
+    let table = new Uint8Array(16384);
+    for (let i = 0; i < 16384; ++i) {


Does this increase startup in a noticeable way?

It adds 10 to 16 ms.

Is that 10-16ms when the first terminal is launched or when the script is loaded?

When the script loads, it is done during import.

Created a follow up 😃 #799

Tyriar · 2017-07-16T17:01:05Z

src/InputHandler.ts

+        }
+        return 1;
+    }
+    let table = new Uint8Array(16384);


A named variable/comment explaining 16384 would be good ((65536-4)/4+1?).

Also this needs to be conditional and fallback to a regular array for browsers without support

- commenting the bit shifts - switching to Uint32Array - conditional for TypedArray - fixing Math.floor slowdown in bisearch

coveralls · 2017-07-16T21:46:49Z

Coverage increased (+0.5%) to 70.616% when pulling 1232c00 on jerch:faster_wcwidth into 48dab49 on sourcelair:master.

coveralls · 2017-07-16T21:57:39Z

Coverage increased (+0.4%) to 70.555% when pulling 6ed5c9e on jerch:faster_wcwidth into 48dab49 on sourcelair:master.

jerch · 2017-07-16T22:01:34Z

@Tyriar Changed the table to Uint32Array, which speeds up the creation time (down to 6ms), it slightly slows down the reading by 20% compared to Uint8Array, dunno why. About the lazy init - I dont know where to hook it into. Is there a sumthing like a terminal-init event?

Tyriar · 2017-07-16T22:36:23Z

@jerch this would mean doing it on either terminal creation (new Terminal()) or on first request of character width (might cause async issues with the way writing works?).

… 127

jerch · 2017-07-17T17:19:53Z

Addressed the lazy loading with the last commit, the lookup table is created at the first call with a char > 127. Benchmarks with node V8 on my machine are now:

old wcwidth ASCII range (51200 times): 6.5MB in 120ms
new wcwidth ASCII range (51200 times): 6.5MB in 60ms
old wcwidth 0-65536 range (100 times): 6.5MB in 850ms
new wcwidth 0-65536 range (100 times): 6.5MB in 100ms

coveralls · 2017-07-17T17:24:11Z

Coverage increased (+0.4%) to 70.584% when pulling 598a729 on jerch:faster_wcwidth into 48dab49 on sourcelair:master.

Tyriar

Looks good to me, I'm planning on pulling this into its own file #800 and doing a clean up so the code style is a little closer to the rest of the codebase 👍

Tyriar · 2017-07-17T18:02:49Z

src/InputHandler.test.ts

+})({nul: 0, control: 0});  // configurable options
+
+describe('wcwidth', () => {
+  it('same as old implementation for BMP and individual higher', () => {


Can't get more confident about no functional changes than this 😉

coveralls · 2017-07-17T18:29:26Z

Coverage increased (+0.4%) to 70.564% when pulling 20048d3 on jerch:faster_wcwidth into 48dab49 on sourcelair:master.

Tyriar · 2017-07-17T20:08:53Z

@jerch CI failed on macOS:

  181 passing (5s)
  1 failing
  1) wcwidth same as old implementation for BMP and individual higher:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

Might need to increase the timeout of that test? https://travis-ci.org/sourcelair/xterm.js/jobs/254546569

jerch · 2017-07-17T21:22:40Z

Hmm yeah seems the travis OSX boxes are pretty busy from time to time and tend timeout longer running tests. The test runs in under a second here...

coveralls · 2017-07-18T20:58:41Z

Coverage increased (+0.4%) to 70.589% when pulling cd5827e on jerch:faster_wcwidth into 48dab49 on sourcelair:master.

faster wcwidth with lookup table

b965d0a

jerch mentioned this pull request Jul 16, 2017

Buffer performance improvements #791

Closed

fixing whitespaces

dd0fcb6

fixing whitespaces

1722248

jerch added 3 commits July 16, 2017 13:57

fixing whitespaces

ba2d0e8

test speedup defensive

6733c6c

test speedup defensive

5d87b54

Tyriar requested changes Jul 16, 2017

View reviewed changes

some fixes

1232c00

- commenting the bit shifts - switching to Uint32Array - conditional for TypedArray - fixing Math.floor slowdown in bisearch

speed test commented out

6ed5c9e

fixes #799 - lookup table is created on first wcwidth call for char >…

598a729

… 127

Tyriar mentioned this pull request Jul 17, 2017

Pull wcwidth into its own file #800

Closed

Tyriar approved these changes Jul 17, 2017

View reviewed changes

Tyriar added this to the 2.9.0 milestone Jul 17, 2017

Merge branch 'master' into faster_wcwidth

20048d3

higher timeout for test case

cd5827e

Tyriar merged commit 05d308a into xtermjs:master Jul 18, 2017

jerch mentioned this pull request Nov 14, 2018

initTable is slow, can we lazily load parts as they're needed? #1785

Closed

jerch mentioned this pull request May 20, 2023

working perf test and API test injection PerBothner/xterm.js#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

faster wcwidth with lookup table #798

faster wcwidth with lookup table #798

jerch commented Jul 16, 2017

coveralls commented Jul 16, 2017

coveralls commented Jul 16, 2017

coveralls commented Jul 16, 2017

coveralls commented Jul 16, 2017 •

edited

coveralls commented Jul 16, 2017 •

edited

Tyriar left a comment

Tyriar Jul 16, 2017

Tyriar Jul 16, 2017

jerch Jul 16, 2017 •

edited

Tyriar Jul 16, 2017

jerch Jul 16, 2017 •

edited

Tyriar Jul 16, 2017

jerch Jul 16, 2017

Tyriar Jul 16, 2017

Tyriar Jul 16, 2017

Tyriar Jul 16, 2017

coveralls commented Jul 16, 2017

coveralls commented Jul 16, 2017 •

edited

jerch commented Jul 16, 2017

Tyriar commented Jul 16, 2017

jerch commented Jul 17, 2017 •

edited

coveralls commented Jul 17, 2017 •

edited

Tyriar left a comment

Tyriar Jul 17, 2017

jerch Jul 17, 2017

coveralls commented Jul 17, 2017

Tyriar commented Jul 17, 2017

jerch commented Jul 17, 2017

coveralls commented Jul 18, 2017 •

edited

faster wcwidth with lookup table #798

faster wcwidth with lookup table #798

Conversation

jerch commented Jul 16, 2017

coveralls commented Jul 16, 2017

coveralls commented Jul 16, 2017

coveralls commented Jul 16, 2017

coveralls commented Jul 16, 2017 • edited

coveralls commented Jul 16, 2017 • edited

Tyriar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerch Jul 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerch Jul 16, 2017 • 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

coveralls commented Jul 16, 2017

coveralls commented Jul 16, 2017 • edited

jerch commented Jul 16, 2017

Tyriar commented Jul 16, 2017

jerch commented Jul 17, 2017 • edited

coveralls commented Jul 17, 2017 • edited

Tyriar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 17, 2017

Tyriar commented Jul 17, 2017

jerch commented Jul 17, 2017

coveralls commented Jul 18, 2017 • edited

coveralls commented Jul 16, 2017 •

edited

coveralls commented Jul 16, 2017 •

edited

jerch Jul 16, 2017 •

edited

jerch Jul 16, 2017 •

edited

coveralls commented Jul 16, 2017 •

edited

jerch commented Jul 17, 2017 •

edited

coveralls commented Jul 17, 2017 •

edited

coveralls commented Jul 18, 2017 •

edited