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

lib: add ASCII fast path to getStringWidth() #29301

Closed
wants to merge 4 commits into from

Conversation

@addaleax
Copy link
Member

commented Aug 25, 2019

A lot of strings that are going to be passed to getStringWidth()
are ASCII strings, for which the calculation is rather easy and
calling into C++ can be skipped.

                                                   confidence improvement accuracy (*)    (**)   (***)
 misc/getstringwidth.js n=100000 type='ascii'            ***    328.99 %      ±21.73% ±29.25% ±38.77%
 misc/getstringwidth.js n=100000 type='emojiseq'                  2.94 %       ±7.66% ±10.19% ±13.26%
 misc/getstringwidth.js n=100000 type='fullwidth'                 4.70 %       ±5.64%  ±7.50%  ±9.76%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
lib: add ASCII fast path to getStringWidth()
A lot of strings that are going to be passed to `getStringWidth()`
are ASCII strings, for which the calculation is rather easy and
calling into C++ can be skipped.

                                                       confidence improvement accuracy (*)    (**)   (***)
     misc/getstringwidth.js n=100000 type='ascii'            ***    328.99 %      ±21.73% ±29.25% ±38.77%
     misc/getstringwidth.js n=100000 type='emojiseq'                  2.94 %       ±7.66% ±10.19% ±13.26%
     misc/getstringwidth.js n=100000 type='fullwidth'                 4.70 %       ±5.64%  ±7.50%  ±9.76%
lib/internal/readline/utils.js Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@lpinca
lpinca approved these changes Aug 29, 2019
@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Aug 30, 2019
@Trott

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Landed in ab841d5

@Trott Trott closed this Sep 3, 2019

Trott added a commit that referenced this pull request Sep 3, 2019
lib: add ASCII fast path to getStringWidth()
A lot of strings that are going to be passed to `getStringWidth()`
are ASCII strings, for which the calculation is rather easy and
calling into C++ can be skipped.

                                                       confidence improvement accuracy (*)    (**)   (***)
     misc/getstringwidth.js n=100000 type='ascii'            ***    328.99 %      ±21.73% ±29.25% ±38.77%
     misc/getstringwidth.js n=100000 type='emojiseq'                  2.94 %       ±7.66% ±10.19% ±13.26%
     misc/getstringwidth.js n=100000 type='fullwidth'                 4.70 %       ±5.64%  ±7.50%  ±9.76%

PR-URL: #29301
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit that referenced this pull request Sep 3, 2019
lib: add ASCII fast path to getStringWidth()
A lot of strings that are going to be passed to `getStringWidth()`
are ASCII strings, for which the calculation is rather easy and
calling into C++ can be skipped.

                                                       confidence improvement accuracy (*)    (**)   (***)
     misc/getstringwidth.js n=100000 type='ascii'            ***    328.99 %      ±21.73% ±29.25% ±38.77%
     misc/getstringwidth.js n=100000 type='emojiseq'                  2.94 %       ±7.66% ±10.19% ±13.26%
     misc/getstringwidth.js n=100000 type='fullwidth'                 4.70 %       ±5.64%  ±7.50%  ±9.76%

PR-URL: #29301
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR referenced this pull request Sep 3, 2019
BridgeAR added a commit that referenced this pull request Sep 4, 2019
lib: add ASCII fast path to getStringWidth()
A lot of strings that are going to be passed to `getStringWidth()`
are ASCII strings, for which the calculation is rather easy and
calling into C++ can be skipped.

                                                       confidence improvement accuracy (*)    (**)   (***)
     misc/getstringwidth.js n=100000 type='ascii'            ***    328.99 %      ±21.73% ±29.25% ±38.77%
     misc/getstringwidth.js n=100000 type='emojiseq'                  2.94 %       ±7.66% ±10.19% ±13.26%
     misc/getstringwidth.js n=100000 type='fullwidth'                 4.70 %       ±5.64%  ±7.50%  ±9.76%

PR-URL: #29301
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
lib: add ASCII fast path to getStringWidth()
A lot of strings that are going to be passed to `getStringWidth()`
are ASCII strings, for which the calculation is rather easy and
calling into C++ can be skipped.

                                                       confidence improvement accuracy (*)    (**)   (***)
     misc/getstringwidth.js n=100000 type='ascii'            ***    328.99 %      ±21.73% ±29.25% ±38.77%
     misc/getstringwidth.js n=100000 type='emojiseq'                  2.94 %       ±7.66% ±10.19% ±13.26%
     misc/getstringwidth.js n=100000 type='fullwidth'                 4.70 %       ±5.64%  ±7.50%  ±9.76%

PR-URL: nodejs#29301
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
lib: add ASCII fast path to getStringWidth()
A lot of strings that are going to be passed to `getStringWidth()`
are ASCII strings, for which the calculation is rather easy and
calling into C++ can be skipped.

                                                       confidence improvement accuracy (*)    (**)   (***)
     misc/getstringwidth.js n=100000 type='ascii'            ***    328.99 %      ±21.73% ±29.25% ±38.77%
     misc/getstringwidth.js n=100000 type='emojiseq'                  2.94 %       ±7.66% ±10.19% ±13.26%
     misc/getstringwidth.js n=100000 type='fullwidth'                 4.70 %       ±5.64%  ±7.50%  ±9.76%

PR-URL: nodejs#29301
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.