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

Speed up require(), phase 1 and 2 #1801

Merged
merged 2 commits into from May 27, 2015
Merged

Conversation

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented May 26, 2015

Replace calls to fs.statSync() with an internal variant that does not
create Error or Stat objects that put strain on the garbage collector.

Replace calls to fs.readFileSync() with an internal variant that does
not create Error objects on failure and is a bit speedier in general.

A secondary benefit is that it improves start-up times in the debugger
because it no longer emits thousands of exception debug events.

On a medium-sized application[0], this commit and its predecessor reduce
start-up times from about 1.5s to 0.5s and reduce the number of start-up
exceptions from ~6100 to 32, half of them internal to the application.

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/712/

R=@trevnorris?

var json = fs.readFileSync(jsonPath, 'utf8');
} catch (e) {
var jsonPath = path.resolve(requestPath, 'package.json');
var json = internalModuleReadFile(jsonPath);

This comment has been minimized.

@bnoordhuis

bnoordhuis May 26, 2015
Author Member

There are two other places where fs.readFileSync could be replaced but that would alter the stack trace in case of error.

I hope and assume no one writes code that depends on the exact stack trace but I decided to let it be for now, just in case.

This comment has been minimized.

@sindresorhus

sindresorhus May 27, 2015

I don't think that should be a concern, especially not when there's potential perf gains. Nobody should depend on the exact stack trace.

This comment has been minimized.

@bnoordhuis

bnoordhuis May 27, 2015
Author Member

I don't really disagree but the other two call sites don't seem to be bottlenecks like the package.json load step is.

@ofrobots
Copy link
Contributor

@ofrobots ofrobots commented May 26, 2015

For posterity, can you provide the missing link for "medium-sized application[0]"?

}
}

fclose(stream);

This comment has been minimized.

@trevnorris

trevnorris May 26, 2015
Contributor

Totally ridiculous case, but what would happen if the file were deleted while being read? I assume fread() would simply return 1, then fclose() would return EBADF?

Likelihood of this happening is near zero.

This comment has been minimized.

@bnoordhuis

bnoordhuis May 26, 2015
Author Member

On UNIX systems, unlinking the file doesn't delete it until the last file descriptor is closed. If another process truncates it, fwrite() simply returns with a small (possibly zero) read count.

This comment has been minimized.

@ChALkeR

ChALkeR May 27, 2015
Member

@bnoordhuis
And if the fs gets unmounted or the connection the the remote fs gets broken?
Probablilty = 0, but still?

This comment has been minimized.

@bnoordhuis

bnoordhuis May 27, 2015
Author Member

This PR won't change the observable behavior of require(), if that is what you're asking.

EDIT: Oh wait, I think I get you now. Yes, fclose() can return an error.

}
uv_fs_req_cleanup(&req);

ASSERT(rc != 0);

This comment has been minimized.

@trevnorris

trevnorris May 26, 2015
Contributor

Why ASSERT() here and not CHECK()?

This comment has been minimized.

@bnoordhuis

bnoordhuis May 26, 2015
Author Member

I can change it to CHECK if you want. It's more to signal to the reader that the return value is never zero.

Actually, I'll just go ahead and remove that. The code in lib/module.js had the concept of a cache and I planned to use 0 as a sentinel value but I ended up removing the cache because it wasn't necessary anymore.

This comment has been minimized.

@trevnorris

trevnorris May 26, 2015
Contributor

Sounds good to me.

@trevnorris
Copy link
Contributor

@trevnorris trevnorris commented May 26, 2015

Just a couple questions, but LGTM.

@bnoordhuis
Copy link
Member Author

@bnoordhuis bnoordhuis commented May 26, 2015

@ofrobots Sorry, it's in the commit log but I forgot to paste it into the description. It's https://github.com/strongloop/loopback-sample-app

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:speed-up-require branch from dedb944 to 6bf70d5 May 27, 2015
@bnoordhuis
Copy link
Member Author

@bnoordhuis bnoordhuis commented May 27, 2015

Incorporated feedback, PTAL.

@trevnorris
Copy link
Contributor

@trevnorris trevnorris commented May 27, 2015

LGTM

bnoordhuis added 2 commits May 26, 2015
Replace calls to fs.statSync() with an internal variant that does not
create Error or Stat objects that put strain on the garbage collector.

A secondary benefit is that it improves start-up times in the debugger
because it no longer emits thousands of exception debug events.

PR-URL: #1801
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Replace calls to fs.readFileSync() with an internal variant that does
not create Error objects on failure and is a bit speedier in general.

A secondary benefit is that it improves start-up times in the debugger
because it no longer emits thousands of exception debug events.

On a medium-sized application[0], this commit and its predecessor reduce
start-up times from about 1.5s to 0.5s and reduce the number of start-up
exceptions from ~6100 to 32, half of them internal to the application.

[0] https://github.com/strongloop/loopback-sample-app

PR-URL: #1801
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis bnoordhuis force-pushed the bnoordhuis:speed-up-require branch from 6bf70d5 to 1bbf8d0 May 27, 2015
@bnoordhuis bnoordhuis closed this May 27, 2015
@bnoordhuis bnoordhuis deleted the bnoordhuis:speed-up-require branch May 27, 2015
@bnoordhuis bnoordhuis merged commit 1bbf8d0 into nodejs:master May 27, 2015
@gx0r

This comment has been minimized.

Copy link

@gx0r gx0r commented on 1bbf8d0 May 28, 2015

Fantastic!

@ghost
Copy link

@ghost ghost commented May 29, 2015

Woh ,iojs just got faster?

@rvagg rvagg mentioned this pull request May 30, 2015
rvagg added a commit to rvagg/io.js that referenced this pull request May 31, 2015
Notable Changes:

* node: Speed-up require() by replacing usage of fs.statSync() and
  fs.readFileSync() with internal variants that are faster for this use-case
  and do not create as many objects for the garbage collector to clean up.
  The primary two benefits are: significant increase in application start-up
  time on typical applications and better start-up time for the debugger by
  eliminating almost all of the thousands of exception events.
  (Ben Noordhuis) nodejs#1801.
* node: Resolution of pre-load modules (-r or --require) now follows the
  standard require() rules rather than just resolving paths, so you can now
  pre-load modules in node_modules. (Ali Ijaz Sheikh) nodejs#1812.
* npm: Upgraded npm to v2.11.0. New hooks for preversion, version, and
  postversion lifecycle events, some SPDX-related license changes and license
  file inclusions. See the release notes for full details.
rvagg added a commit to rvagg/io.js that referenced this pull request May 31, 2015
PR-URL: nodejs#1808

Notable Changes:

* node: Speed-up require() by replacing usage of fs.statSync() and
  fs.readFileSync() with internal variants that are faster for this use-case
  and do not create as many objects for the garbage collector to clean up.
  The primary two benefits are: significant increase in application start-up
  time on typical applications and better start-up time for the debugger by
  eliminating almost all of the thousands of exception events.
  (Ben Noordhuis) nodejs#1801.
* node: Resolution of pre-load modules (-r or --require) now follows the
  standard require() rules rather than just resolving paths, so you can now
  pre-load modules in node_modules. (Ali Ijaz Sheikh) nodejs#1812.
* npm: Upgraded npm to v2.11.0. New hooks for preversion, version, and
  postversion lifecycle events, some SPDX-related license changes and license
  file inclusions. See the release notes for full details.
andrewdeandrade added a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Replace calls to fs.statSync() with an internal variant that does not
create Error or Stat objects that put strain on the garbage collector.

A secondary benefit is that it improves start-up times in the debugger
because it no longer emits thousands of exception debug events.

PR-URL: nodejs/node#1801
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
andrewdeandrade added a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Replace calls to fs.readFileSync() with an internal variant that does
not create Error objects on failure and is a bit speedier in general.

A secondary benefit is that it improves start-up times in the debugger
because it no longer emits thousands of exception debug events.

On a medium-sized application[0], this commit and its predecessor reduce
start-up times from about 1.5s to 0.5s and reduce the number of start-up
exceptions from ~6100 to 32, half of them internal to the application.

[0] https://github.com/strongloop/loopback-sample-app

PR-URL: nodejs/node#1801
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Jun 16, 2015

It looks stat at least one module out there was monkey-patching fs and relied on require using that: tschaub/mock-fs#43

But I guess monkey-patching was never supported, because supporting it means freezing everything.

targos added a commit to targos/node that referenced this pull request Jun 16, 2015
nodejs#1801 introduced internal fs methods to speed up require.
The methods do not call path._makeLong like their counterpart
from the fs module. This brings back the old behaviour.

Fixes: nodejs#1990
Fixes: nodejs#1980
Fixes: nodejs#1849
piscisaureus added a commit that referenced this pull request Jun 16, 2015
#1801 introduced internal fs methods
to speed up require. The methods do not call path._makeLong like their
counterpart from the fs module. This brings back the old behaviour.

Fixes: #1990
Fixes: #1980
Fixes: #1849

PR-URL: https://github.com/nodejs/io.js/pull/1991/files
Reviewed-By: Bert Belder <bertbelder@gmail.com>
rvagg added a commit that referenced this pull request Jun 23, 2015
PR-URL: #1996

Notable changes

* module: The number of syscalls made during a require() have been
  significantly reduced again (see #1801 from v2.2.0 for previous
  work), which should lead to a performance improvement
  (Pierre Inglebert) #1920.
* npm:
  - Upgrade to v2.11.2 (Rebecca Turner) #1956.
  - Upgrade to v2.11.3 (Forrest L Norvell) #2018.
* zlib: A bug was discovered where the process would abort if the
  final part of a zlib decompression results in a buffer that would
  exceed the maximum length of 0x3fffffff bytes (~1GiB). This was
  likely to only occur during buffered decompression (rather than
  streaming). This is now fixed and will instead result in a thrown
  RangeError (Michaël Zasso) #1811.
rvagg added a commit that referenced this pull request Jun 23, 2015
PR-URL: #1996

Notable changes

* module: The number of syscalls made during a require() have been
  significantly reduced again (see #1801 from v2.2.0 for previous
  work), which should lead to a performance improvement
  (Pierre Inglebert) #1920.
* npm:
  - Upgrade to v2.11.2 (Rebecca Turner) #1956.
  - Upgrade to v2.11.3 (Forrest L Norvell) #2018.
* zlib: A bug was discovered where the process would abort if the
  final part of a zlib decompression results in a buffer that would
  exceed the maximum length of 0x3fffffff bytes (~1GiB). This was
  likely to only occur during buffered decompression (rather than
  streaming). This is now fixed and will instead result in a thrown
  RangeError (Michaël Zasso) #1811.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants