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

src: clean up MaybeStackBuffer usage in i18n #11464

Closed
wants to merge 5 commits into from

Conversation

TimothyGu
Copy link
Member

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×106 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

@nodejs-github-bot 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 TimothyGu added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 20, 2017
@TimothyGu
Copy link
Member Author

@mscdex
Copy link
Contributor

mscdex commented Feb 20, 2017

/cc @nodejs/intl

@TimothyGu TimothyGu changed the title Clean up MaybeStackBuffer usage in i18n src: clean up MaybeStackBuffer usage in i18n Feb 20, 2017
if (sizeof(T) > 1 && IsBigEndian()) {
SPREAD_BUFFER_ARG(ret.ToLocalChecked(), retbuf);
SwapBytes16(retbuf_data, retbuf_length);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

src/util.h Outdated
if (storage > capacity()) {
T* allocatedPtr = IsAllocated() ? buf_ : nullptr;
buf_ = Realloc<T>(allocatedPtr, storage);
capacity_ = storage;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@TimothyGu
Copy link
Member Author

@TimothyGu
Copy link
Member Author

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

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

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo nits.

src/node_i18n.cc Outdated
if (buf->IsAllocated()) {
MaybeLocal<Object> ret = Buffer::New(isolate, buf->out(), len);
if (!ret.IsEmpty()) buf->Release();
template<typename T>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny style nit: space before <.

// underlying buffer. However, |buf| itself can be reused even after this call,
// but its capacity, if increased through AllocateSufficientStorage, is not
// guaranteed to stay the same.
template<typename T>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

MaybeStackBuffer<T>* buf) {
v8::MaybeLocal<v8::Object> ret;
char* src = reinterpret_cast<char*>(buf->out());
const size_t len_in_bytes = buf->length() * sizeof(T);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/util.h Outdated
@@ -300,58 +301,81 @@ class MaybeStackBuffer {
return buf_[index];
}

// Size of the buffer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat superfluous comment.

src/util.h Outdated
// There can only be 1 call to AllocateSufficientStorage or Invalidate
// per instance.
// Actual maximum capacity of the buffer with which SetLength() can be called
// without first calling AllocateSufficientStorage()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you punctuate the comment?

src/util.h Outdated
// Make sure enough space for `storage` entries is available.
// This method can be called multiple times throughout the lifetime of the
// buffer, but once this has been called Invalidate() cannot be used.
// Contents of the buffer in the range [0, length()) are preserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/util.h Outdated
if (storage > capacity()) {
bool was_allocated = IsAllocated();
T* allocated_ptr = was_allocated ? buf_ : nullptr;
buf_ = Realloc<T>(allocated_ptr, storage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/util.h Outdated
buf_ = Realloc<T>(allocated_ptr, storage);
capacity_ = storage;
if (!was_allocated && length_ > 0)
memcpy(buf_, buf_st_, length_ * sizeof(T));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sizeof(buf_[0])?

src/util.h Outdated
length_ = 0;
buf_ = nullptr;
}

bool IsAllocated() {
return buf_ != buf_st_;
// If the buffer is stored in the heap rather than on the stack
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you punctuate the comments here and below?

EXPECT_FALSE(buf.IsAllocated()); \
EXPECT_GT(buf.capacity(), buf.length()); \
free(rawbuf); \
} while (0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

@bnoordhuis, comments addressed. PTAL.

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

return ret;

if (buf->IsAllocated())
buf->Release();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done in the if block above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/util.h Outdated
// length_ stores how much memory was allocated.
CHECK_LE(length, length_);
// capacity_ stores how much memory was allocated.
CHECK_LE(length, capacity());
length_ = length;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment also needs to be updated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Will fix.

TimothyGu added a commit to TimothyGu/node that referenced this pull request Feb 25, 2017
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 added a commit to TimothyGu/node that referenced this pull request Feb 25, 2017
- 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>
TimothyGu added a commit to TimothyGu/node that referenced this pull request Feb 25, 2017
- 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>
TimothyGu added a commit to TimothyGu/node that referenced this pull request Feb 25, 2017
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
Copy link
Member Author

Landed in 289e532...d4e1eaf.

@TimothyGu TimothyGu closed this Feb 25, 2017
@TimothyGu TimothyGu deleted the idna branch February 25, 2017 01:56
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 25, 2017
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>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 25, 2017
- 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>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 25, 2017
- 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>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 25, 2017
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>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

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

TimothyGu added a commit to TimothyGu/node that referenced this pull request Nov 28, 2017
- 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>
@TimothyGu TimothyGu mentioned this pull request Nov 28, 2017
4 tasks
MylesBorins pushed a commit that referenced this pull request Jan 18, 2018
- 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>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
- 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>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
- 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>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants