FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: speed up module loading by bnoordhuis · Pull Request #9132 · nodejs/node · GitHub

/ node Public

src: speed up module loading - #9132

Merged
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:speed-up-module-loading
Oct 24, 2016
ConversationCommits (2)ChecksFiles changed
Merged

src: speed up module loading#9132
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:speed-up-module-loading

Conversation

bnoordhuis commented Oct 17, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

bnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. labels Oct 17, 2016
nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 17, 2016

jbergstroem commented Oct 17, 2016

Copy link
Copy Markdown
Member

FreeBSD fails looks unrelated.

mscdex commented Oct 17, 2016

Copy link
Copy Markdown
Contributor

Out of curiosity, how much of a speed up is this?

bnoordhuis commented Oct 17, 2016

Copy link
Copy Markdown
Member Author

Nothing spectacular, it's on the order of 50-100 us per file on my system.

jasnell approved these changes Oct 17, 2016
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

jbergstroem commented Oct 17, 2016

Copy link
Copy Markdown
Member

(the pending status of windows-fanned is a bug we have with reporting build updates from jenkins)

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

bnoordhuis commented Oct 24, 2016

Copy link
Copy Markdown
Member Author

One more CI run because the previous one had infrastructure issues: https://ci.nodejs.org/job/node-test-pull-request/4648/

src: speed up module loading, skip EOF read
35e8896 
Stop reading from disk when we read fewer bytes than requested because
the next read will be the zero-sized EOF.

PR-URL: nodejs#9132
Reviewed-By: James M Snell <jasnell@gmail.com>
src: speed up module loading, don't resize buffer
d52f5dc 
Don't bother shrinking the read buffer on the final read because we
dispose it immediately afterwards.  Avoids some unnecessary memory
allocation and copying.

PR-URL: nodejs#9132
Reviewed-By: James M Snell <jasnell@gmail.com>
bnoordhuis force-pushed the speed-up-module-loading branch from cfcc70f to d52f5dc Compare October 24, 2016 20:31
bnoordhuis closed this Oct 24, 2016
bnoordhuis deleted the speed-up-module-loading branch October 24, 2016 20:32
bnoordhuis merged commit d52f5dc into nodejs:master Oct 24, 2016
evanlucas pushed a commit that referenced this pull request Nov 2, 2016
src: speed up module loading, skip EOF read
362c307 
Stop reading from disk when we read fewer bytes than requested because
the next read will be the zero-sized EOF.

PR-URL: #9132
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 2, 2016
src: speed up module loading, don't resize buffer
8b53f3c 
Don't bother shrinking the read buffer on the final read because we
dispose it immediately afterwards.  Avoids some unnecessary memory
allocation and copying.

PR-URL: #9132
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas mentioned this pull request Nov 3, 2016
Merged

MylesBorins commented Nov 17, 2016

Copy link
Copy Markdown
Contributor

hey @bnoordhuis should this be backported? Assuming it needs more time to bake either way

bnoordhuis commented Nov 18, 2016

Copy link
Copy Markdown
Member Author

It could be back-ported but it doesn't need to be, it's not a bug fix.

MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 8, 2017
MylesBorins added dont-land-on-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Nov 14, 2017

MylesBorins commented Nov 14, 2017

Copy link
Copy Markdown
Contributor

setting don't land on this for v6.x for now. Please feel free to let me know if you think we should re consider

This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes

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.

6 participants

Back | FazBrowse Home | New Git URL