net: use icu's punycode implementation #7355

jasnell · 2016-06-21T21:10:08Z

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

net, intl

Description of change

ICU has a punycode implementation built in. Use it instead of the javascript implementation because it's much faster.

The punycode module is currently only used in one location in core (in the url module). With this change if ICU is present, the ICU implementation would be used with the punycode impl becoming the fallback. Eventually, it would be great to see if we could deprecate the punycode module entirely, with an eye towards eventually removing it.

This includes an update to the small-icu dataset to support the IDNA implementation. The additional data adds < 1 mb of data.

Benchmarks (bigger numbers are better):

net/punycode.js method="punycode" n="1024" val="افغانستا.icom.museum": 27593.19811
net/punycode.js method="punycode" n="1024" val="الجزائر.icom.museum": 30275.51786
net/punycode.js method="punycode" n="1024" val="österreich.icom.museum": 34711.86323
net/punycode.js method="punycode" n="1024" val="বাংলাদেশ.icom.museum": 26096.37590
net/punycode.js method="punycode" n="1024" val="беларусь.icom.museum": 26819.07514
net/punycode.js method="punycode" n="1024" val="belgië.icom.museum": 39787.58249
net/punycode.js method="punycode" n="1024" val="българия.icom.museum": 26728.08628
net/punycode.js method="punycode" n="1024" val="تشادر.icom.museum": 33340.09143
net/punycode.js method="punycode" n="1024" val="中国.icom.museum": 40001.78289
net/punycode.js method="punycode" n="1024" val="القمر.icom.museum": 34623.28045
net/punycode.js method="punycode" n="1024" val="κυπρος.icom.museum": 32423.57551
net/punycode.js method="punycode" n="1024" val="českárepublika.icom.museum": 30496.93111
net/punycode.js method="punycode" n="1024" val="مصر.icom.museum": 38439.50043
net/punycode.js method="punycode" n="1024" val="ελλάδα.icom.museum": 32494.77561
net/punycode.js method="punycode" n="1024" val="magyarország.icom.museum": 35283.22541
net/punycode.js method="punycode" n="1024" val="ísland.icom.museum": 38816.50008
net/punycode.js method="punycode" n="1024" val="भारत.icom.museum": 37098.76096
net/punycode.js method="punycode" n="1024" val="ايران.icom.museum": 31919.15278
net/punycode.js method="punycode" n="1024" val="éire.icom.museum": 36912.57326
net/punycode.js method="punycode" n="1024" val="איקו״ם.ישראל.museum": 15457.49763
net/punycode.js method="punycode" n="1024" val="日本.icom.museum": 40124.44062
net/punycode.js method="punycode" n="1024" val="الأردن.icom.museum": 30370.73983

net/punycode.js method="icu" n="1024" val="افغانستا.icom.museum": 265435.02052
net/punycode.js method="icu" n="1024" val="الجزائر.icom.museum": 327985.94529
net/punycode.js method="icu" n="1024" val="österreich.icom.museum": 300055.76232
net/punycode.js method="icu" n="1024" val="বাংলাদেশ.icom.museum": 235323.31677
net/punycode.js method="icu" n="1024" val="беларусь.icom.museum": 229452.87589
net/punycode.js method="icu" n="1024" val="belgië.icom.museum": 354082.67000
net/punycode.js method="icu" n="1024" val="българия.icom.museum": 256043.71947
net/punycode.js method="icu" n="1024" val="تشادر.icom.museum": 357039.62547
net/punycode.js method="icu" n="1024" val="中国.icom.museum": 385073.29554
net/punycode.js method="icu" n="1024" val="القمر.icom.museum": 351974.70182
net/punycode.js method="icu" n="1024" val="κυπρος.icom.museum": 297908.24193
net/punycode.js method="icu" n="1024" val="českárepublika.icom.museum": 268331.79121
net/punycode.js method="icu" n="1024" val="مصر.icom.museum": 338442.24685
net/punycode.js method="icu" n="1024" val="ελλάδα.icom.museum": 340001.77306
net/punycode.js method="icu" n="1024" val="magyarország.icom.museum": 301679.21012
net/punycode.js method="icu" n="1024" val="ísland.icom.museum": 370667.99875
net/punycode.js method="icu" n="1024" val="भारत.icom.museum": 205594.87089
net/punycode.js method="icu" n="1024" val="ايران.icom.museum": 302680.25733
net/punycode.js method="icu" n="1024" val="éire.icom.museum": 397259.52996
net/punycode.js method="icu" n="1024" val="איקו״ם.ישראל.museum": 202994.92850
net/punycode.js method="icu" n="1024" val="日本.icom.museum": 407493.58416
net/punycode.js method="icu" n="1024" val="الأردن.icom.museum": 326276.22915

cc @srl295 @mscdex @ChALkeR @trevnorris

srl295 · 2016-06-21T21:25:25Z

does the binary size change much? i don't think it should.

jasnell · 2016-06-21T21:36:15Z

With the code, no. Not sure about the data files. I need to track down
exactly which additional data files are needed
On Jun 21, 2016 2:25 PM, "Steven R. Loomis" [email protected]
wrote:

does the binary size change much? i don't think it should.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7355 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eQuKo2eIYt7PM1-WSnnxx2cc3Skhks5qOFbMgaJpZM4I7LOq
.

srl295 · 2016-06-21T21:55:48Z

Maybe uts46? maybe the stringprep dir? https://github.com/nodejs/node/blob/master/tools/icu/icu_small.json#L37

jasnell · 2016-06-21T22:06:54Z

Yeah, likely both. I'll play around with adding those into the small-icu subset and see what happens ;-)

sindresorhus · 2016-06-21T22:52:18Z

// @mathiasbynens

trevnorris · 2016-06-22T05:41:26Z

src/node_i18n.cc

+
+  if (status == U_BUFFER_OVERFLOW_ERROR) {
+    status = U_ZERO_ERROR;
+    dest = static_cast<char*>(malloc(len));


You'll at least want to do a CHECK_NE(dest, nullptr)

Better yet, use MaybeStackBuffer.

bnoordhuis · 2016-06-22T09:34:07Z

High-level comment: is punycode prevalent enough to warrant optimizing for? I'm inclined to say it isn't.

srl295 · 2016-06-22T14:23:19Z

@bnoordhuis Maybe this is what you meant, but the function is called every time url.parse() is called, isn't it?

> url.parse('http://mañana.com').host
'xn--maana-pta.com'

bnoordhuis · 2016-06-22T14:55:22Z

Let me rephrase: does punycode.toASCII() show up in the profile of a non-contrived benchmark? Maybe I don't call url.parse() enough, but I've never seen it.

jasnell · 2016-06-22T15:34:58Z

@bnoordhuis ... it's not used extensively by our code, no, but it is used pretty much every time url.parse() is called which is every time someone does something like http.get('http://example.org') (see https://github.com/nodejs/node/blob/master/lib/url.js#L312). This is the only use of the punycode module in core.

Ultimately what I'd really like is for us to deprecate the punycode module and eventually pull it back out of core entirely, relying solely on the faster ICU implementation. What that would mean is not supporting IDNA's in builds that do not include ICU but if those are not including ICU then internationalization is likely not a requirement and it's not likely to be any issue (and if it is, they can rely on the userland module to get it).

jasnell · 2016-06-22T16:20:16Z

@bnoordhuis ... to give an idea, below is a comparison of url.parse benchmarks in v6.2.2 against master with this patch applied:

url/url-parse.js type="one" n="250000": /Users/james/.nvm/versions/node/v6.2.2/bin/node: 249020 ./node: 370670 ... -32.82%
url/url-parse.js type="two" n="250000": /Users/james/.nvm/versions/node/v6.2.2/bin/node: 299860 ./node: 493920 ... -39.29%
url/url-parse.js type="three" n="250000": /Users/james/.nvm/versions/node/v6.2.2/bin/node: 238360 ./node: 345490 . -31.01%
url/url-parse.js type="four" n="250000": /Users/james/.nvm/versions/node/v6.2.2/bin/node: 727730 ./node: 730270 ... -0.35%
url/url-parse.js type="five" n="250000": /Users/james/.nvm/versions/node/v6.2.2/bin/node: 716050 ./node: 609270 ... 17.52%
url/url-parse.js type="six" n="250000": /Users/james/.nvm/versions/node/v6.2.2/bin/node: 271480 ./node: 421830 ... -35.64%

The "five" case is an exception and I believe that something else unrelated to this change is going on there.. but the perf boost for the typical use cases is significant.

jasnell · 2016-06-22T16:33:25Z

@bnoordhuis @saghul @trevnorris @srl295 .. updated! PTAL

trevnorris · 2016-06-22T18:48:42Z

src/node_i18n.cc

+  }
+
+  if (U_FAILURE(status)) {
+    env->ThrowError("Cannot convert name to ASCII");


does this give a simple yes/no on failure, or is there additional information that may be useful to the error message?

In this particular case the possible failures are rare and non-interesting.

Looks like you can test for info.errors & UIDNA_ERROR_DOMAIN_NAME_TOO_LONG, maybe other values. The status itself is likely to be something like out of memory, invalid parameters, etc. You could ThrowError including u_errorName(status) to return the ICU status code - that would be good. I don't know if ThrowError takes a parameter.

trevnorris · 2016-06-22T18:58:18Z

what a pain. so http.get() uses punycode automatically but not encodeURI()? so a request like http://見.香港/見 would properly convert the domain but not the path. At least in a way that node would like. So to be full proof a user would have to do:

// url.parse() automatically runs toASCII() on the
// domain, but isn't documented to do so.
const o = url.parse('http://見.香港/見');
o.pathname = encodeURI(o.pathname);
const url = url.format(o);

Anyway, not directly anything to do with this PR.

In terms of performance, I think punycode.js could be made noticeably faster for node. Since it's not written for node to begin with (as shown by comments like // Avoid split(regex) for IE8 compatibility.). My main question is the need to use full-icu data for this to work, and so how much actual use it will see.

Strictly from the code perspective, the updates LGTM.

jasnell · 2016-06-22T19:33:49Z

This only needs full-icu currently. There are a couple of minor data files currently missing in the small-icu subset that would need to be added but those are minimal. Those can be added to small-icu without too much impact.

jasnell · 2016-06-22T19:34:07Z

And I'm working on a better URL impl to deal with the other issues ;-)

mathiasbynens · 2016-06-23T13:19:14Z

@trevnorris I think punycode.js could be made noticeably faster for node.

How?

bnoordhuis · 2016-06-23T15:35:08Z

@mathiasbynens By playing to V8's strengths while avoiding its weaknesses. :-)

For example, .toASCII() calls String#split() twice and is heavy on the map-over-substrings idiom. It would probably be faster to:

Scan for the split character and only split when found; saves allocating an array in the common case.
Iterate rather than map; V8 is not great at lowering/inlining callbacks.
Try to avoid concatenating strings too much; cons strings eventually have to be flattened.

There is probably more but that is what stood out from a quick look.

Fishrock123 · 2016-06-24T11:20:31Z

lib/url.js

+  }
+}
+
+const {toASCII} = importPunycode();


should we have a spacing rule for destructuring before we start using it in full? // @Trott

In other places we have spaces around the name.

jasnell · 2016-06-28T18:38:30Z

CI had a failure on smartos... green everywhere else. Small tweak and running again: https://ci.nodejs.org/job/node-test-pull-request/3108/

jasnell · 2016-06-28T18:56:44Z

@trevnorris @bnoordhuis @srl295 ... CI is good. This now no-longer requires full-icu data. PTAL

trevnorris · 2016-06-28T20:42:23Z

src/node_i18n.cc

+
+  if (len < 0) {
+    env->ThrowError("Cannot convert name to ASCII");
+    return;


should be able to put these two on the same line. git grep "return.*Throw" shows that's how it's done in 260 other places.

bnoordhuis · 2016-06-28T20:50:32Z

LGTM with style nits.

jasnell · 2016-06-28T20:58:26Z

@bnoordhuis @trevnorris .. updated to address the nits

ICU has a punycode implementation built in. Use it instead of the javascript implementation because it's much faster.

jasnell · 2016-06-29T23:29:44Z

Final CI before landing: https://ci.nodejs.org/job/node-test-pull-request/3134/

jasnell · 2016-06-30T00:09:54Z

CI is green.

PR-URL: #7355 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

ICU has a punycode implementation built in. Use it instead of the javascript implementation because it's much faster. PR-URL: #7355 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

jasnell · 2016-06-30T00:15:47Z

Landed in 7de59ef and 3d6a01e

PR-URL: #7355 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

ICU has a punycode implementation built in. Use it instead of the javascript implementation because it's much faster. PR-URL: #7355 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins · 2016-07-11T23:31:30Z

@jasnell @srl295 I've set this to don't land as the icu doesn't come packed with v4.x

please feel free to update if I'm incorrect here

srl295 · 2016-07-11T23:37:42Z

@thealphanerd Correct and correct.

jasnell added wip Issues and PRs that are still a work in progress. net Issues and PRs related to the net subsystem. i18n-api Issues and PRs related to the i18n implementation. performance Issues and PRs related to the performance of Node.js. labels Jun 21, 2016

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. url Issues and PRs related to the legacy built-in url module. labels Jun 21, 2016

trevnorris reviewed Jun 22, 2016
View reviewed changes

mathiasbynens mentioned this pull request Jun 23, 2016

Improve V8-specific performance mathiasbynens/punycode.js#49

Open

Fishrock123 reviewed Jun 24, 2016
View reviewed changes

trevnorris reviewed Jun 28, 2016
View reviewed changes

jasnell added 2 commits June 28, 2016 16:07

deps: update icu-small to include punycode datafiles

87e3741

net: use icu's punycode implementation

9e96196

ICU has a punycode implementation built in. Use it instead of the javascript implementation because it's much faster.

jasnell force-pushed the icu-punycode branch from 03e91bd to 9e96196 Compare June 28, 2016 23:08

jasnell added a commit that referenced this pull request Jun 30, 2016

deps: update icu-small to include punycode datafiles

3d6a01e

PR-URL: #7355 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

jasnell closed this Jun 30, 2016

mhdawson mentioned this pull request Jun 30, 2016

AIX: build failure #7500

Closed

Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016

deps: update icu-small to include punycode datafiles

12b1993

PR-URL: #7355 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Fishrock123 mentioned this pull request Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the dont-land-on-v4.x label Jul 11, 2016

srl295 mentioned this pull request Jul 27, 2016

[WIP] Update V8 to 5.2 #7824

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: use icu's punycode implementation #7355

net: use icu's punycode implementation #7355

jasnell commented Jun 21, 2016 •

edited

Loading

srl295 commented Jun 21, 2016

jasnell commented Jun 21, 2016

srl295 commented Jun 21, 2016

jasnell commented Jun 21, 2016

sindresorhus commented Jun 21, 2016

trevnorris Jun 22, 2016

bnoordhuis Jun 22, 2016

bnoordhuis commented Jun 22, 2016

srl295 commented Jun 22, 2016

bnoordhuis commented Jun 22, 2016

jasnell commented Jun 22, 2016

jasnell commented Jun 22, 2016

jasnell commented Jun 22, 2016

trevnorris Jun 22, 2016

jasnell Jun 22, 2016

srl295 Jun 24, 2016

trevnorris commented Jun 22, 2016

jasnell commented Jun 22, 2016

jasnell commented Jun 22, 2016

mathiasbynens commented Jun 23, 2016

bnoordhuis commented Jun 23, 2016

Fishrock123 Jun 24, 2016

bnoordhuis Jun 28, 2016

jasnell commented Jun 28, 2016

jasnell commented Jun 28, 2016

trevnorris Jun 28, 2016

bnoordhuis commented Jun 28, 2016

jasnell commented Jun 28, 2016

jasnell commented Jun 29, 2016

jasnell commented Jun 30, 2016

jasnell commented Jun 30, 2016

MylesBorins commented Jul 11, 2016

srl295 commented Jul 11, 2016

net: use icu's punycode implementation #7355

net: use icu's punycode implementation #7355

Conversation

jasnell commented Jun 21, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

srl295 commented Jun 21, 2016

jasnell commented Jun 21, 2016

srl295 commented Jun 21, 2016

jasnell commented Jun 21, 2016

sindresorhus commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Jun 22, 2016

srl295 commented Jun 22, 2016

bnoordhuis commented Jun 22, 2016

jasnell commented Jun 22, 2016

jasnell commented Jun 22, 2016

jasnell commented Jun 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Jun 22, 2016

jasnell commented Jun 22, 2016

jasnell commented Jun 22, 2016

mathiasbynens commented Jun 23, 2016

bnoordhuis commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jun 28, 2016

jasnell commented Jun 28, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jun 28, 2016

jasnell commented Jun 28, 2016

jasnell commented Jun 29, 2016

jasnell commented Jun 30, 2016

jasnell commented Jun 30, 2016

MylesBorins commented Jul 11, 2016

srl295 commented Jul 11, 2016

jasnell commented Jun 21, 2016 •

edited

Loading