src: clean up MaybeStackBuffer usage in i18n by TimothyGu · Pull Request #11464 · nodejs/node

TimothyGu · 2017-02-20T06:56:22Z

This PR can roughly be divided into two parts. It first cleans up MaybeStackBuffer itself:

Make IsAllocated() work for invalidated buffers
Add capacity() method for finding out the actual capacity, not the current size, of the buffer
Allow multiple calls to AllocateSufficientStorage() and Invalidate()
Add more descriptive comments describing the purpose of the methods
Assert buffer is malloc'd in Release()

Then, it aims to make the usage of MaybeStackBuffer in node_i18n more orthodox:

Templatize MaybeStackBuffer<T>-toBuffer conversion function (AsBuffer()) and move it to Buffer namespace as a private constructor
- The original plan was to move it to MaybeStackBuffer, but there are multiple problems in doing that due to header/class dependency. On the other hand, Buffer is as good as a home for the function as any.
Use the new MaybeStackBuffer::storage() instead of defining its own version of kStackStorageSize (which may get out of sync with the official one in MaybeStackBuffer)
Instead of requiring a len parameter to AsBuffer(), use MaybeStackBuffer::SetLength() uniformly
If possible, avoid double conversion in ToASCII()/ToUnicode()

That last bullet point brings a nice performance enhancement for url.domainToASCII()/domainToUnicode() with short strings (n reduced to 1×10⁶ since this method is not as susceptible to JIT fluctuations):

                                                                 improvement confidence      p.value
 url/whatwg-url-idna.js n=1000000 to="ascii" input="all"             45.22 %        *** 1.498146e-11
 url/whatwg-url-idna.js n=1000000 to="ascii" input="empty"            1.51 %            3.121468e-01
 url/whatwg-url-idna.js n=1000000 to="ascii" input="none"             9.04 %        *** 5.434629e-06
 url/whatwg-url-idna.js n=1000000 to="ascii" input="nonstring"        0.45 %            8.529213e-01
 url/whatwg-url-idna.js n=1000000 to="ascii" input="some"            42.67 %        *** 9.040307e-15
 url/whatwg-url-idna.js n=1000000 to="unicode" input="all"           58.61 %        *** 3.859159e-16
 url/whatwg-url-idna.js n=1000000 to="unicode" input="empty"         -1.54 %            6.379120e-01
 url/whatwg-url-idna.js n=1000000 to="unicode" input="none"          10.67 %        *** 5.320423e-08
 url/whatwg-url-idna.js n=1000000 to="unicode" input="nonstring"     -0.90 %            5.238741e-01
 url/whatwg-url-idna.js n=1000000 to="unicode" input="some"          56.80 %        *** 8.087484e-16

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

src

TimothyGu · 2017-02-20T07:03:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6505/

mscdex · 2017-02-20T07:06:14Z

/cc @nodejs/intl

addaleax · 2017-02-20T12:57:07Z

src/node_internals.h

I think I would prefer it if we did the SwapBytes16() at the call side and kept Buffer::New()s job restricted to turning the memory block into a Buffer instance?

addaleax · 2017-02-20T13:01:25Z

src/util.h

Right now, if the buffer wasn’t allocated before, the previous contents are lost (but not if it was malloc()’ed). I guess this okay and intended behaviour, but maybe note that in the comment, too?

PR updated. Now contents up to length_ - 1 are preserved if buf_ was on the stack.

srl295

the ICU changes seem good to me. (Side note: I misread MaybeStackBuffer as ICU's MaybeStackArray at first… )

addaleax

Thanks!

TimothyGu · 2017-02-20T18:30:31Z

Updated CI: https://ci.nodejs.org/job/node-test-pull-request/6514/

TimothyGu · 2017-02-22T01:04:51Z

Rebased and changed static_cast<unsigned>(0) to simply 0U.

Updated CI: https://ci.nodejs.org/job/node-test-pull-request/6535/

bnoordhuis

LGTM modulo nits.

bnoordhuis · 2017-02-22T14:27:46Z

src/node_i18n.cc

Tiny style nit: space before <.

bnoordhuis · 2017-02-22T14:34:58Z

src/node_internals.h

bnoordhuis · 2017-02-22T14:43:39Z

src/node_internals.h

sizeof(buf[0]) should work too and is arguably a little more Obviously Correct. Here, you don't know what T is without looking at the surrounding code.

bnoordhuis · 2017-02-22T14:44:17Z

src/util.h

Somewhat superfluous comment.

bnoordhuis · 2017-02-22T14:44:37Z

src/util.h

Can you punctuate the comment?

bnoordhuis · 2017-02-22T14:47:04Z

src/util.h

s/are/is/? 'Contents' is indefinite and therefore singular (I think? Not 100% sure.)

bnoordhuis · 2017-02-22T14:49:28Z

src/util.h

The explicit <T> shouldn't be necessary. Not a big deal, though.

bnoordhuis · 2017-02-22T14:50:20Z

src/util.h

sizeof(buf_[0])?

bnoordhuis · 2017-02-22T14:50:54Z

src/util.h

Can you punctuate the comments here and below?

bnoordhuis · 2017-02-22T14:54:07Z

test/cctest/util.cc

Consider putting this in a template function rather than a macro. Rule of thumb: template when you can, macro when you must.

jasnell

Overall LGTM with @bnoordhuis' comments addressed

- Add IsInvalidated() method - Add capacity() method for finding out the actual capacity, not the current size, of the buffer - Make IsAllocated() work for invalidated buffers - Allow multiple calls to AllocateSufficientStorage() and Invalidate() - Assert buffer is malloc'd in Release() - Assert buffer has not been invalidated in AllocateSufficientStorage() - Add more descriptive comments describing the purpose of the methods - Add cctest for MaybeStackBuffer

- Templatize AsBuffer() and create a generic version for inclusion in the Buffer class - Use MaybeStackBuffer::storage() - If possible, avoid double conversion in ToASCII()/ToUnicode() - More descriptive assertion error in tests

TimothyGu · 2017-02-23T01:20:06Z

@bnoordhuis, comments addressed. PTAL.

Updated CI: https://ci.nodejs.org/job/node-test-pull-request/6560/

thefourtheye · 2017-02-23T02:27:33Z

src/node_internals.h

+    return ret;
+
+  if (buf->IsAllocated())
+    buf->Release();


Can this be done in the if block above?

The ret.IsEmpty() check has to happen before calling buf->Release(), so unfortunately not.

thefourtheye · 2017-02-23T02:30:02Z

src/util.h

  }

  void SetLengthAndZeroTerminate(size_t length) {
    // length_ stores how much memory was allocated.


This comment also needs to be updated?

Good catch. Will fix.

PR-URL: nodejs#11464 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

- Add IsInvalidated() method - Add capacity() method for finding out the actual capacity, not the current size, of the buffer - Make IsAllocated() work for invalidated buffers - Allow multiple calls to AllocateSufficientStorage() and Invalidate() - Assert buffer is malloc'd in Release() - Assert buffer has not been invalidated in AllocateSufficientStorage() - Add more descriptive comments describing the purpose of the methods - Add cctest for MaybeStackBuffer PR-URL: nodejs#11464 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

- Templatize AsBuffer() and create a generic version for inclusion in the Buffer class - Use MaybeStackBuffer::storage() - If possible, avoid double conversion in ToASCII()/ToUnicode() - More descriptive assertion error in tests PR-URL: nodejs#11464 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#11464 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

TimothyGu · 2017-02-25T01:55:57Z

Landed in 289e532...d4e1eaf.

PR-URL: nodejs#11464 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

- Add IsInvalidated() method - Add capacity() method for finding out the actual capacity, not the current size, of the buffer - Make IsAllocated() work for invalidated buffers - Allow multiple calls to AllocateSufficientStorage() and Invalidate() - Assert buffer is malloc'd in Release() - Assert buffer has not been invalidated in AllocateSufficientStorage() - Add more descriptive comments describing the purpose of the methods - Add cctest for MaybeStackBuffer PR-URL: nodejs#11464 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

- Templatize AsBuffer() and create a generic version for inclusion in the Buffer class - Use MaybeStackBuffer::storage() - If possible, avoid double conversion in ToASCII()/ToUnicode() - More descriptive assertion error in tests PR-URL: nodejs#11464 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#11464 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2017-03-07T05:13:54Z

Parts of this should likely be backported to v6.x (and possibly v4.x).

- Add IsInvalidated() method - Add capacity() method for finding out the actual capacity, not the current size, of the buffer - Make IsAllocated() work for invalidated buffers - Allow multiple calls to AllocateSufficientStorage() and Invalidate() - Assert buffer is malloc'd in Release() - Assert buffer has not been invalidated in AllocateSufficientStorage() - Add more descriptive comments describing the purpose of the methods - Add cctest for MaybeStackBuffer PR-URL: nodejs#11464 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

- Add IsInvalidated() method - Add capacity() method for finding out the actual capacity, not the current size, of the buffer - Make IsAllocated() work for invalidated buffers - Allow multiple calls to AllocateSufficientStorage() and Invalidate() - Assert buffer is malloc'd in Release() - Assert buffer has not been invalidated in AllocateSufficientStorage() - Add more descriptive comments describing the purpose of the methods - Add cctest for MaybeStackBuffer Backport-PR-URL: #17365 PR-URL: #11464 Reviewed-By: Steven R Loomis <srloomis@us.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. labels Feb 20, 2017

TimothyGu added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 20, 2017

TimothyGu changed the title ~~Clean up MaybeStackBuffer usage in i18n~~ src: clean up MaybeStackBuffer usage in i18n Feb 20, 2017

watilde mentioned this pull request Feb 20, 2017

2017 March Intl WG meeting: Mar 27, 2017 nodejs/Intl#40

Closed

addaleax reviewed Feb 20, 2017

View reviewed changes

srl295 approved these changes Feb 20, 2017

View reviewed changes

addaleax approved these changes Feb 20, 2017

View reviewed changes

TimothyGu force-pushed the idna branch from 5d9b206 to 70822ea Compare February 22, 2017 01:02

bnoordhuis approved these changes Feb 22, 2017

View reviewed changes

jasnell approved these changes Feb 22, 2017

View reviewed changes

TimothyGu added 4 commits February 22, 2017 16:36

src: don't assume v8::Local is using-declared

62539ca

src, i18n: cleanup usage of MaybeStackBuffer

199d115

- Templatize AsBuffer() and create a generic version for inclusion in the Buffer class - Use MaybeStackBuffer::storage() - If possible, avoid double conversion in ToASCII()/ToUnicode() - More descriptive assertion error in tests

benchmark: add url.domainTo*()

b5acb67

TimothyGu force-pushed the idna branch from 70822ea to b5acb67 Compare February 23, 2017 01:18

thefourtheye reviewed Feb 23, 2017

View reviewed changes

util: update comments

0995c12

TimothyGu closed this Feb 25, 2017

TimothyGu deleted the idna branch February 25, 2017 01:56

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

jasnell added dont-land-on-v4.x labels Mar 7, 2017

TimothyGu mentioned this pull request Nov 28, 2017

Backport WHATWG URL to 6.x #17365

Closed

4 tasks

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

Uh oh!

Conversation

TimothyGu commented Feb 20, 2017

Checklist

Affected core subsystem(s)

Uh oh!

TimothyGu commented Feb 20, 2017

Uh oh!

mscdex commented Feb 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srl295 left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Feb 20, 2017

Uh oh!

TimothyGu commented Feb 22, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Feb 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Feb 25, 2017

Uh oh!

jasnell commented Mar 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants