src: use a std::vector for preload_modules by sam-github · Pull Request #12241 · nodejs/node

sam-github · 2017-04-05T18:49:35Z

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

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

sam-github · 2017-04-05T18:50:23Z

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

kkoopa

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

kkoopa · 2017-04-06T10:34:13Z

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) {


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

kkoopa · 2017-04-06T10:38:28Z

src/node.cc

      }
      args_consumed += 1;
-      local_preload_modules[preload_module_count++] = module;
+      preload_modules.push_back(module);


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

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

sam-github · 2017-04-06T18:58:23Z

@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).

kkoopa · 2017-04-06T19:13:53Z

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)

kkoopa · 2017-04-06T19:19:49Z

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)

addaleax · 2017-04-10T14:11:05Z

Landed in cecdf7c

A dynamically allocated array was being used, simplify the memory management by using std::vector. PR-URL: #12241 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

italoacasas · 2017-04-10T22:19:30Z

cc @sam-github

A dynamically allocated array was being used, simplify the memory management by using std::vector. PR-URL: nodejs#12241 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

gibfahn · 2017-06-18T00:10:58Z

Backport PR: #12677

A dynamically allocated array was being used, simplify the memory management by using std::vector. PR-URL: nodejs#12241 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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 <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 5, 2017

richardlau approved these changes Apr 5, 2017

View reviewed changes

mscdex added the module Issues and PRs related to the module subsystem. label Apr 5, 2017

sam-github mentioned this pull request Apr 5, 2017

src: allow CLI args in env with NODE_OPTIONS #12028

Merged

4 tasks

jasnell approved these changes Apr 6, 2017

View reviewed changes

danbev approved these changes Apr 6, 2017

View reviewed changes

kkoopa reviewed Apr 6, 2017

View reviewed changes

cjihrig approved these changes Apr 6, 2017

View reviewed changes

addaleax approved these changes Apr 9, 2017

View reviewed changes

addaleax closed this Apr 10, 2017

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 added the backport-to-v7.x label Apr 10, 2017

sam-github deleted the use-vector-for-preload-modules branch April 11, 2017 03:41

sam-github mentioned this pull request May 16, 2017

(7.x backport) NODE_OPTIONS, and one of its dependant refactors #13063

Merged

4 tasks

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

sam-github removed the backport-requested-v7.x label Jun 5, 2017

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the backport-requested-v6.x label Jun 18, 2017

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Oct 16, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: use a std::vector for preload_modules#12241

src: use a std::vector for preload_modules#12241
sam-github wants to merge 1 commit intonodejs:masterfrom
sam-github:use-vector-for-preload-modules

sam-github commented Apr 5, 2017

Uh oh!

sam-github commented Apr 5, 2017

Uh oh!

kkoopa left a comment

Uh oh!

kkoopa Apr 6, 2017

Uh oh!

kkoopa Apr 6, 2017

Uh oh!

sam-github Apr 6, 2017

Uh oh!

sam-github commented Apr 6, 2017

Uh oh!

kkoopa commented Apr 6, 2017 via email

Uh oh!

kkoopa commented Apr 6, 2017 via email

Uh oh!

addaleax commented Apr 10, 2017

Uh oh!

italoacasas commented Apr 10, 2017

Uh oh!

gibfahn commented Jun 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Uh oh!

Conversation

sam-github commented Apr 5, 2017

Checklist

Affected core subsystem(s)

Uh oh!

sam-github commented Apr 5, 2017

Uh oh!

kkoopa left a comment

Choose a reason for hiding this comment

Uh oh!

kkoopa Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

kkoopa Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

sam-github Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

sam-github commented Apr 6, 2017

Uh oh!

kkoopa commented Apr 6, 2017 via email

Uh oh!

kkoopa commented Apr 6, 2017 via email

Uh oh!

addaleax commented Apr 10, 2017

Uh oh!

italoacasas commented Apr 10, 2017

Uh oh!

gibfahn commented Jun 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants