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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse node's implementation of Module._nodeModulePaths #6213

Merged
merged 5 commits into from Jun 24, 2016

Conversation

Projects
None yet
2 participants
@kevinsawicki
Copy link
Contributor

kevinsawicki commented Jun 23, 2016

Pull request #2976 added support for require not looking outside of the current application when loading modules by patching Module._nodeModulePaths and pulling in some code from node's own module.js implementation.

Node's implementation got some great improvements via nodejs/node#5172 and this pull request seeks to use those improvements via post-processing the results of the default implementation instead of reimplementing the method fully.

  • Add specs for previous behavior
  • Use default Module._nodeModulePaths implementations and filter results in patched version
  • Profile it 馃弴

This was profiled by calling Module._nodeModulePaths using 1,000 paths that were all inside the app's resources path so the filtering is happening on every call.

It looks to be a 50-75% speed improvement, results below.

var path = require('path')
var Module = require('module')
var paths = require('./paths')

module.exports = function () {
  var start = Date.now()
  var count = 0
  var total = 0
  for (var i = 0; i < paths.length; i++) {
    count++
    total += Module._nodeModulePaths(paths[i]).length
  }

  var time = Date.now() - start
  console.log(`${paths.length} paths resolved to ${total} search paths in ${time}ms`)

}

Before

screen shot 2016-06-23 at 3 47 37 pm

### After

screen shot 2016-06-23 at 3 48 23 pm

@kevinsawicki kevinsawicki changed the title Reuse node implementation of Module._nodeModulePaths Reuse node's implementation of Module._nodeModulePaths Jun 23, 2016

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Jun 24, 2016

馃憤

@zcbenz zcbenz merged commit 552c9b7 into master Jun 24, 2016

8 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #3516004 succeeded in 43s
Details
electron-linux-ia32 Build #3516005 succeeded in 36s
Details
electron-linux-x64 Build #3516006 succeeded in 122s
Details
electron-mas-x64 Build #1695 succeeded in 6 min 2 sec
Details
electron-osx-x64 Build #1703 succeeded in 7 min 1 sec
Details
electron-win-ia32 Build #698 succeeded in 6 min 18 sec
Details
electron-win-x64 Build #688 succeeded in 6 min 16 sec
Details

@zcbenz zcbenz deleted the reset-search-paths branch Jun 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.