src: fix AliasedBuffer memory attribution in heap snapshots #46817
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before it depended on util-inl.h. Fix it by moving MaybeStackBuffer::AllocateSufficientStorage() into util-inl.h
Drive-by: Replace the SFINAE with a static_assert because we don't have (or need) an implementation for non-scalar AliasedBufferBase otherwise. Add forward declarations to memory_tracker.h now that aliased-buffer.h no longer includes util-inl.h.
Before the AliasedBuffers were represented solely by the TypedArrays event though they also retain additional memory themselves. Make the accounting more accurate by implementing MemoryRetainer in AliasedBuffer.
|
Review requested:
|
nodejs-github-bot
added
c++
addaleax
approved these changes
Feb 24, 2023
16 tasks
|
Fixed linter error |
addaleax
approved these changes
Feb 28, 2023
15 tasks
joyeecheung
added a commit
that referenced
this pull request
Mar 1, 2023
Before it depended on util-inl.h. Fix it by moving MaybeStackBuffer::AllocateSufficientStorage() into util-inl.h PR-URL: #46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
joyeecheung
added a commit
that referenced
this pull request
Mar 1, 2023
Drive-by: Replace the SFINAE with a static_assert because we don't have (or need) an implementation for non-scalar AliasedBufferBase otherwise. Add forward declarations to memory_tracker.h now that aliased-buffer.h no longer includes util-inl.h. PR-URL: #46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
joyeecheung
added a commit
that referenced
this pull request
Mar 1, 2023
Before the AliasedBuffers were represented solely by the TypedArrays event though they also retain additional memory themselves. Make the accounting more accurate by implementing MemoryRetainer in AliasedBuffer. PR-URL: #46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
|
Landed in d0608c2...3646a66 |
This was referenced Mar 2, 2023
kvakil
added a commit
to kvakil/node
that referenced
this pull request
Mar 13, 2023
Compiling with clang 14 on Ubuntu was failing with the following error: ```bash $ CXX=clang++ CC=clang ./configure --ninja $ make <snip> [7/14] LINK cctest FAILED: cctest clang++ <snip> /usr/bin/ld: obj/src/inspector/libnode.node_string.o: in function `node::inspector::protocol::StringUtil::StringViewToUtf8[abi:cxx11](v8_inspector::StringView)': node_string.cc:(.text+0x2ce): undefined reference to `node::MaybeStackBuffer<char, 1024ul>::AllocateSufficientStorage(unsigned long)' /usr/bin/ld: obj/src/inspector/libnode.node_string.o: in function `node::inspector::protocol::StringUtil::fromUTF16[abi:cxx11](unsigned short const*, unsigned long)': node_string.cc:(.text+0xad9): undefined reference to `node::MaybeStackBuffer<char, 1024ul>::AllocateSufficientStorage(unsigned long)' clang: error: linker command failed with exit code 1 (use -v to see invocation) ``` I bisected it to the referenced PR. This change includes the header file which defines `node::MaybeStackBuffer::AllocateSufficientStorage`. Refs: nodejs#46817
kvakil
added a commit
to kvakil/node
that referenced
this pull request
Mar 13, 2023
Compiling with clang 14 on Ubuntu was failing with a linker error that `node::MaybeStackBuffer::AllocateSufficientStorage` was undefined in `src/inspector/node_string.cc`. I bisected it to the referenced PR. Include `util-inl.h` which defines `node::MaybeStackBuffer::AllocateSufficientStorage`. Refs: nodejs#46817
targos
pushed a commit
that referenced
this pull request
Mar 13, 2023
Before it depended on util-inl.h. Fix it by moving MaybeStackBuffer::AllocateSufficientStorage() into util-inl.h PR-URL: #46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
targos
pushed a commit
that referenced
this pull request
Mar 13, 2023
Drive-by: Replace the SFINAE with a static_assert because we don't have (or need) an implementation for non-scalar AliasedBufferBase otherwise. Add forward declarations to memory_tracker.h now that aliased-buffer.h no longer includes util-inl.h. PR-URL: #46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
targos
pushed a commit
that referenced
this pull request
Mar 13, 2023
Before the AliasedBuffers were represented solely by the TypedArrays event though they also retain additional memory themselves. Make the accounting more accurate by implementing MemoryRetainer in AliasedBuffer. PR-URL: #46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
targos
pushed a commit
that referenced
this pull request
Mar 14, 2023
Before it depended on util-inl.h. Fix it by moving MaybeStackBuffer::AllocateSufficientStorage() into util-inl.h PR-URL: #46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
|
Should land after the permission model to avoid conflicts |
nodejs-github-bot
pushed a commit
that referenced
this pull request
Mar 15, 2023
Compiling with clang 14 on Ubuntu was failing with a linker error that `node::MaybeStackBuffer::AllocateSufficientStorage` was undefined in `src/inspector/node_string.cc`. I bisected it to the referenced PR. Include `util-inl.h` which defines `node::MaybeStackBuffer::AllocateSufficientStorage`. Refs: #46817 PR-URL: #47057 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
targos
pushed a commit
that referenced
this pull request
Mar 18, 2023
Compiling with clang 14 on Ubuntu was failing with a linker error that `node::MaybeStackBuffer::AllocateSufficientStorage` was undefined in `src/inspector/node_string.cc`. I bisected it to the referenced PR. Include `util-inl.h` which defines `node::MaybeStackBuffer::AllocateSufficientStorage`. Refs: #46817 PR-URL: #47057 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
danielleadams
pushed a commit
that referenced
this pull request
Apr 11, 2023
Before it depended on util-inl.h. Fix it by moving MaybeStackBuffer::AllocateSufficientStorage() into util-inl.h PR-URL: #46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
danielleadams
added
the
backport-requested-v18.x
danielleadams
pushed a commit
that referenced
this pull request
Jul 6, 2023
Compiling with clang 14 on Ubuntu was failing with a linker error that `node::MaybeStackBuffer::AllocateSufficientStorage` was undefined in `src/inspector/node_string.cc`. I bisected it to the referenced PR. Include `util-inl.h` which defines `node::MaybeStackBuffer::AllocateSufficientStorage`. Refs: #46817 PR-URL: #47057 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
targos
pushed a commit
that referenced
this pull request
Nov 10, 2023
Drive-by: Replace the SFINAE with a static_assert because we don't have (or need) an implementation for non-scalar AliasedBufferBase otherwise. Add forward declarations to memory_tracker.h now that aliased-buffer.h no longer includes util-inl.h. PR-URL: #46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
targos
pushed a commit
that referenced
this pull request
Nov 10, 2023
Before the AliasedBuffers were represented solely by the TypedArrays event though they also retain additional memory themselves. Make the accounting more accurate by implementing MemoryRetainer in AliasedBuffer. PR-URL: #46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
sercher
added a commit
to sercher/graaljs
that referenced
this pull request
Apr 25, 2024
Drive-by: Replace the SFINAE with a static_assert because we don't have (or need) an implementation for non-scalar AliasedBufferBase otherwise. Add forward declarations to memory_tracker.h now that aliased-buffer.h no longer includes util-inl.h. PR-URL: nodejs/node#46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
sercher
added a commit
to sercher/graaljs
that referenced
this pull request
Apr 25, 2024
Before the AliasedBuffers were represented solely by the TypedArrays event though they also retain additional memory themselves. Make the accounting more accurate by implementing MemoryRetainer in AliasedBuffer. PR-URL: nodejs/node#46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
sercher
added a commit
to sercher/graaljs
that referenced
this pull request
Apr 25, 2024
Drive-by: Replace the SFINAE with a static_assert because we don't have (or need) an implementation for non-scalar AliasedBufferBase otherwise. Add forward declarations to memory_tracker.h now that aliased-buffer.h no longer includes util-inl.h. PR-URL: nodejs/node#46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
sercher
added a commit
to sercher/graaljs
that referenced
this pull request
Apr 25, 2024
Before the AliasedBuffers were represented solely by the TypedArrays event though they also retain additional memory themselves. Make the accounting more accurate by implementing MemoryRetainer in AliasedBuffer. PR-URL: nodejs/node#46817 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
src: make util.h self-containted
Before it depended on util-inl.h. Fix it by moving
MaybeStackBuffer::AllocateSufficientStorage() into
util-inl.h
src: move AliasedBuffer implementation to -inl.h
Drive-by: Replace the SFINAE with a static_assert because we don't
have (or need) an implementation for non-scalar AliasedBufferBase
otherwise. Add forward declarations to memory_tracker.h now that
aliased-buffer.h no longer includes util-inl.h.
src: fix AliasedBuffer memory attribution in heap snapshots
Before the AliasedBuffers were represented solely by the TypedArrays
event though they also retain additional memory themselves.
Make the accounting more accurate by implementing MemoryRetainer in
AliasedBuffer.
Before:
After: