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: use a std::vector for preload_modules #12241

Closed

Conversation

@sam-github
Copy link
Member

commented Apr 5, 2017

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

src: use a std::vector for preload_modules
A dynamically allocated array was being used, simplify the memory
management by using std::vector.
@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2017

@mscdex mscdex added the module label Apr 5, 2017

@sam-github sam-github referenced this pull request Apr 5, 2017
4 of 4 tasks complete
@jasnell
jasnell approved these changes Apr 6, 2017
@danbev
danbev approved these changes Apr 6, 2017
@kkoopa
Copy link

left a comment

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

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

This comment has been minimized.

Copy link
@kkoopa

kkoopa Apr 6, 2017

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

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

This comment has been minimized.

Copy link
@kkoopa

kkoopa Apr 6, 2017

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

This comment has been minimized.

Copy link
@sam-github

sam-github Apr 6, 2017

Author Member

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

@cjihrig
cjihrig approved these changes Apr 6, 2017
@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2017

@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

This comment has been minimized.

Copy link

commented Apr 6, 2017

@kkoopa

This comment has been minimized.

Copy link

commented Apr 6, 2017

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

Landed in cecdf7c

@addaleax addaleax closed this Apr 10, 2017

addaleax added a commit that referenced this pull request Apr 10, 2017
src: use a std::vector for preload_modules
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

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

@sam-github sam-github deleted the sam-github:use-vector-for-preload-modules branch Apr 11, 2017

sam-github added a commit to sam-github/node that referenced this pull request May 18, 2017
src: use a std::vector for preload_modules
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>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

Backport PR: #12677

sam-github added a commit to sam-github/node that referenced this pull request Jul 10, 2017
src: use a std::vector for preload_modules
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>
sam-github added a commit to sam-github/node that referenced this pull request Jul 24, 2017
src: use a std::vector for preload_modules
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>
sam-github added a commit to sam-github/node that referenced this pull request Jul 24, 2017
src: use a std::vector for preload_modules
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>
sam-github added a commit to sam-github/node that referenced this pull request Sep 25, 2017
src: use a std::vector for preload_modules
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>
sam-github added a commit to sam-github/node that referenced this pull request Sep 25, 2017
src: use a std::vector for preload_modules
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>
sam-github added a commit to sam-github/node that referenced this pull request Oct 5, 2017
src: use a std::vector for preload_modules
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>
sam-github added a commit to sam-github/node that referenced this pull request Oct 10, 2017
src: use a std::vector for preload_modules
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>
sam-github added a commit to sam-github/node that referenced this pull request Oct 10, 2017
src: use a std::vector for preload_modules
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>
sam-github added a commit to sam-github/node that referenced this pull request Oct 10, 2017
src: use a std::vector for preload_modules
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>
sam-github added a commit to sam-github/node that referenced this pull request Oct 11, 2017
src: use a std::vector for preload_modules
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>
MylesBorins added a commit that referenced this pull request Oct 16, 2017
src: use a std::vector for preload_modules
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>
@MylesBorins MylesBorins referenced this pull request Oct 17, 2017
MylesBorins added a commit that referenced this pull request Oct 25, 2017
src: use a std::vector for preload_modules
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>
@MylesBorins MylesBorins referenced this pull request Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.