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

Fix frame vector size #62

Merged
merged 1 commit into from Nov 24, 2022
Merged

Fix frame vector size #62

merged 1 commit into from Nov 24, 2022

Conversation

nsavoire
Copy link

push_back is replaced by direct indexing since std::vector<uintptr_t> output(n); already creates a a vector with n elements (set to 0).

`push_back` is replaced by direct indexing since `std::vector<uintptr_t> output(n);`
already creates a a vector with n elements (set to 0).
@nsavoire nsavoire added the semver-patch Bug or security fixes, mainly label Nov 22, 2022
@github-actions
Copy link

Overall package size

Self size: 1.44 MB
Deduped: 8.3 MB
No deduping: 8.31 MB

Dependency sizes

name version self size total size
protobufjs 7.1.2 2.76 MB 6.55 MB
source-map 0.7.4 226 kB 226 kB
split 1.0.1 12.29 kB 24.82 kB
findit2 2.2.3 14.78 kB 14.78 kB
p-limit 3.1.0 7.75 kB 13.78 kB
delay 5.0.0 11.17 kB 11.17 kB
pify 5.0.0 8.87 kB 8.87 kB
node-gyp-build 3.9.0 8.81 kB 8.81 kB

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@@ -31,7 +31,7 @@ std::vector<uintptr_t> MakeFrames(v8::Isolate* isolate) {
std::vector<uintptr_t> output(n);

Choose a reason for hiding this comment

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

indeed we already resize it here

Copy link
Collaborator

@Qard Qard left a comment

Choose a reason for hiding this comment

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

LGTM. Just curious, is there any perf difference between the two if the size is already set? I pretty much just used push_back out of habit. 😅

@nsavoire
Copy link
Author

nsavoire commented Nov 24, 2022

LGTM. Just curious, is there any perf difference between the two if the size is already set? I pretty much just used push_back out of habit. 😅

The issue here is not performance: std::vector<uintptr_t> output(n); creates a vector with n elements (all set to zero).
Then each push_back will add another element, so in the end output has 2n elements with the first n elements being 0.
To insert n elements in a vector, you can:

  • Create an empty vector (std::vector<uintptr_t> output;) and then use push_back. Vector will resize itself by doubling its capacity accordingly.
  • You can improve the previous if you know the number of items in advance, by using output.reserve(n); to pre-allocate the vector to its final capacity.
  • Construct the vector with n elements (as done in this PR), the elements will be value initialized (ie. set to zero in the case of ints) and then set the value of each elements by indexing the vector. For ints, this approach might be a little bit faster than the previous one, but for elements that are costlier to value initialize, the previous one would be better.

@nsavoire nsavoire merged commit c8cd16c into main Nov 24, 2022
@nsavoire nsavoire deleted the nsavoire/fix_vector_size branch November 24, 2022 14:43
@Qard
Copy link
Collaborator

Qard commented Nov 28, 2022

Ah, good to know. For some reason I was thinking it worked like output.reserve(n). Good catch! 😅

nsavoire added a commit that referenced this pull request Feb 16, 2023
`push_back` is replaced by direct indexing since `std::vector<uintptr_t> output(n);`
already creates a a vector with n elements (set to 0).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Bug or security fixes, mainly
Projects
None yet
3 participants