GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: use a std::vector for preload_modules by sam-github · Pull Request #12241 · nodejs/node

Conversation

@sam-github

@sam-github sam-github commented Apr 5, 2017

Copy link
Copy Markdown
Contributor

A dynamically allocated array was being used, simplify the memory
management by using std::vector.

Factored out of #12028, its unrelated and easily backportable.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

src

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sam-github
src: use a std::vector for preload_modules
b64ef6b 
A dynamically allocated array was being used, simplify the memory
management by using std::vector.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 5, 2017
@sam-github

sam-github commented Apr 5, 2017

Copy link
Copy Markdown
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request/7234/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
richardlau
richardlau approved these changes Apr 5, 2017
View reviewed changes
@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Apr 5, 2017
@sam-github sam-github mentioned this pull request Apr 5, 2017
src: allow CLI args in env with NODE_OPTIONS #12028
Merged
4 tasks done
jasnell
jasnell approved these changes Apr 6, 2017
View reviewed changes
danbev
danbev approved these changes Apr 6, 2017
View reviewed changes
kkoopa
kkoopa reviewed Apr 6, 2017
View reviewed changes

@kkoopa kkoopa left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Left some minor comments which likely have no actual performance impact.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/node.cc
if (!preload_modules.empty()) {
Local<Array> array = Array::New(env->isolate());
for (unsigned int i = 0; i < preload_module_count; ++i) {
for (unsigned int i = 0; i < preload_modules.size(); ++i) {

@kkoopa kkoopa Apr 6, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A do-while loop would avoid one unnecessary bounds check. There is at least one element in the vector.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/node.cc
}
args_consumed += 1;
local_preload_modules[preload_module_count++] = module;
preload_modules.push_back(module);

@kkoopa kkoopa Apr 6, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It should be possible to reserve space in the vector before inserting elements.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sam-github sam-github Apr 6, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How? This is only inserting one element, I don't know how many -r options were passed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
cjihrig
cjihrig approved these changes Apr 6, 2017
View reviewed changes
@sam-github

sam-github commented Apr 6, 2017

Copy link
Copy Markdown
Contributor Author

@kkoopa I appreciate the feedback, but I think readability and minimization of the diff outweights the cost of one additional integer comparison, which is anyhow dwarfed by the cost of actually requiring the module(s).
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kkoopa

kkoopa commented Apr 6, 2017 via email

Copy link
Copy Markdown
reserve argc, optionally do a shrink_to_fit at the end
…
On April 6, 2017 9:57:09 PM GMT+03:00, Sam Roberts ***@***.***> wrote: sam-github commented on this pull request. > @@ -3717,7 +3711,7 @@ static void ParseArgs(int* argc, exit(9); } args_consumed += 1; - local_preload_modules[preload_module_count++] = module; + preload_modules.push_back(module); How? This is only inserting one element, I don't know how many `-r` options were passed. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #12241 (comment)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kkoopa

kkoopa commented Apr 6, 2017 via email

Copy link
Copy Markdown
Readability is unaffected and essentially subjective, so that is not the best reason. Keeping the diff size down is however a valid reason.
…
On April 6, 2017 9:59:01 PM GMT+03:00, Sam Roberts ***@***.***> wrote: @kkoopa I appreciate the feedback, but I think readability and minimization of the diff outweights the cost of one additional integer comparison, which is anyhow dwarfed by the cost of actually requiring the module(s). -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #12241 (comment)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Apr 9, 2017
View reviewed changes
@addaleax

addaleax commented Apr 10, 2017

Copy link
Copy Markdown
Member

Landed in cecdf7c
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax closed this Apr 10, 2017
addaleax pushed a commit that referenced this pull request Apr 10, 2017
@sam-github @addaleax
src: use a std::vector for preload_modules
cecdf7c 
A dynamically allocated array was being used, simplify the memory
management by using std::vector.

PR-URL: #12241
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
This was referenced Apr 10, 2017
src: guard bundled_ca/openssl_ca with HAVE_OPENSSL #12302
Closed
src: use option parser for expose_internals #12245
Merged
@italoacasas italoacasas added the backport-to-v7.x label Apr 10, 2017
@italoacasas

italoacasas commented Apr 10, 2017

Copy link
Copy Markdown

cc @sam-github
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sam-github
sam-github deleted the use-vector-for-preload-modules branch April 11, 2017 03:41
@sam-github sam-github mentioned this pull request May 16, 2017
(7.x backport) NODE_OPTIONS, and one of its dependant refactors #13063
Merged
4 tasks
sam-github added a commit to sam-github/node that referenced this pull request May 18, 2017
@sam-github
src: use a std::vector for preload_modules
d62f314 
A dynamically allocated array was being used, simplify the memory
management by using std::vector.

PR-URL: nodejs#12241
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell jasnell mentioned this pull request May 11, 2017
8.0.0 Release Proposal #12220
Closed
@sam-github sam-github removed the backport-requested-v7.x label Jun 5, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Auditing for 6.11.1 nodejs/Release#230
Closed
2 of 3 tasks
@gibfahn gibfahn added the backport-requested-v6.x label Jun 18, 2017
@gibfahn

gibfahn commented Jun 18, 2017

Copy link
Copy Markdown
Member

Backport PR: #12677
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sam-github added a commit to sam-github/node that referenced this pull request Oct 11, 2017
@sam-github
src: use a std::vector for preload_modules
f83ccb4 
A dynamically allocated array was being used, simplify the memory
management by using std::vector.

PR-URL: nodejs#12241
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@sam-github @MylesBorins
src: use a std::vector for preload_modules
69c1329 
A dynamically allocated array was being used, simplify the memory
management by using std::vector.

Backport-PR-URL: #12677
PR-URL: #12241
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Oct 16, 2017
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
v6.12.0 proposal #16263
Merged
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@sam-github @MylesBorins
src: use a std::vector for preload_modules
8f42148 
A dynamically allocated array was being used, simplify the memory
management by using std::vector.

Backport-PR-URL: #12677
PR-URL: #12241
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
v4.8.6 proposal #16500
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
@richardlau richardlau richardlau approved these changes
+2 more reviewers
@kkoopa kkoopa kkoopa left review comments
@danbev danbev danbev approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

@sam-github @kkoopa @addaleax @italoacasas @gibfahn @danbev @jasnell @cjihrig @richardlau @mscdex @MylesBorins @nodejs-github-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.