GeezerVison

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

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
.

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

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

// @mathiasbynens

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

@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'

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.

@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).

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

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

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.

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.

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

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

How?

@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:

1. Scan for the split character and only split when found; saves allocating an array in the common case.
2. Iterate rather than map; V8 is not great at lowering/inlining callbacks.
3. 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.

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

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

LGTM with style nits.

@bnoordhuis @trevnorris .. updated to address the nits

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

CI is green.

Landed in 7de59ef and 3d6a01e

@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

@thealphanerd Correct and correct.