Skip to content

Commit edb8e9a

Browse files
committed
fix: perf regression on hot string munging path
This doesn't change any functionality, but it optimizes a few extremely hot code paths for the input typically encountered during a large npm project installation. `String.normalize()` is cached, and trailing-slash removal is done with a single `String.slice()`, rather than multiple slices and `String.length` comparisons. It is extremely rare that any code path is ever hot enough for this kind of thing to be relevant enough to justify this sort of microoptimization, but these two issues resulted in a 25-50% install time increase in some cases, which is fairly dramatic. Fix: npm/cli#3676 PR-URL: #286 Credit: @isaacs Close: #286 Reviewed-by: @wraithgar
1 parent a9d9b05 commit edb8e9a

6 files changed

+40
-28
lines changed

Diff for: lib/normalize-unicode.js

+11
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
// warning: extremely hot code path.
2+
// This has been meticulously optimized for use
3+
// within npm install on large package trees.
4+
// Do not edit without careful benchmarking.
5+
const normalizeCache = Object.create(null)
6+
const {hasOwnProperty} = Object.prototype
7+
module.exports = s => {
8+
if (!hasOwnProperty.call(normalizeCache, s))
9+
normalizeCache[s] = s.normalize('NFKD')
10+
return normalizeCache[s]
11+
}

Diff for: lib/path-reservations.js

+4-5
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
// while still allowing maximal safe parallelization.
88

99
const assert = require('assert')
10-
const normPath = require('./normalize-windows-path.js')
10+
const normalize = require('./normalize-unicode.js')
1111
const stripSlashes = require('./strip-trailing-slashes.js')
1212
const { join } = require('path')
1313

@@ -28,7 +28,7 @@ module.exports = () => {
2828
const getDirs = path => {
2929
const dirs = path.split('/').slice(0, -1).reduce((set, path) => {
3030
if (set.length)
31-
path = normPath(join(set[set.length - 1], path))
31+
path = join(set[set.length - 1], path)
3232
set.push(path || '/')
3333
return set
3434
}, [])
@@ -116,9 +116,8 @@ module.exports = () => {
116116
// So, we just pretend that every path matches every other path here,
117117
// effectively removing all parallelization on windows.
118118
paths = isWindows ? ['win32 parallelization disabled'] : paths.map(p => {
119-
return stripSlashes(normPath(join(p)))
120-
.normalize('NFKD')
121-
.toLowerCase()
119+
// don't need normPath, because we skip this entirely for windows
120+
return normalize(stripSlashes(join(p))).toLowerCase()
122121
})
123122

124123
const dirs = new Set(

Diff for: lib/strip-trailing-slashes.js

+10-21
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,13 @@
1-
// this is the only approach that was significantly faster than using
2-
// str.replace(/\/+$/, '') for strings ending with a lot of / chars and
3-
// containing multiple / chars.
4-
const batchStrings = [
5-
'/'.repeat(1024),
6-
'/'.repeat(512),
7-
'/'.repeat(256),
8-
'/'.repeat(128),
9-
'/'.repeat(64),
10-
'/'.repeat(32),
11-
'/'.repeat(16),
12-
'/'.repeat(8),
13-
'/'.repeat(4),
14-
'/'.repeat(2),
15-
'/',
16-
]
17-
1+
// warning: extremely hot code path.
2+
// This has been meticulously optimized for use
3+
// within npm install on large package trees.
4+
// Do not edit without careful benchmarking.
185
module.exports = str => {
19-
for (const s of batchStrings) {
20-
while (str.length >= s.length && str.slice(-1 * s.length) === s)
21-
str = str.slice(0, -1 * s.length)
6+
let i = str.length - 1
7+
let slashesStart = -1
8+
while (i > -1 && str.charAt(i) === '/') {
9+
slashesStart = i
10+
i--
2211
}
23-
return str
12+
return slashesStart === -1 ? str : str.slice(0, slashesStart)
2413
}

Diff for: lib/unpack.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ const pathReservations = require('./path-reservations.js')
1717
const stripAbsolutePath = require('./strip-absolute-path.js')
1818
const normPath = require('./normalize-windows-path.js')
1919
const stripSlash = require('./strip-trailing-slashes.js')
20+
const normalize = require('./normalize-unicode.js')
2021

2122
const ONENTRY = Symbol('onEntry')
2223
const CHECKFS = Symbol('checkFs')
@@ -101,8 +102,7 @@ const uint32 = (a, b, c) =>
101102
// Note that on windows, we always drop the entire cache whenever a
102103
// symbolic link is encountered, because 8.3 filenames are impossible
103104
// to reason about, and collisions are hazards rather than just failures.
104-
const cacheKeyNormalize = path => stripSlash(normPath(path))
105-
.normalize('NFKD')
105+
const cacheKeyNormalize = path => normalize(stripSlash(normPath(path)))
106106
.toLowerCase()
107107

108108
const pruneCache = (cache, abs) => {

Diff for: test/normalize-unicode.js

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
const t = require('tap')
2+
const normalize = require('../lib/normalize-unicode.js')
3+
4+
// café
5+
const cafe1 = Buffer.from([0x63, 0x61, 0x66, 0xc3, 0xa9]).toString()
6+
7+
// cafe with a `
8+
const cafe2 = Buffer.from([0x63, 0x61, 0x66, 0x65, 0xcc, 0x81]).toString()
9+
10+
t.equal(normalize(cafe1), normalize(cafe2), 'matching unicodes')
11+
t.equal(normalize(cafe1), normalize(cafe2), 'cached')
12+
t.equal(normalize('foo'), 'foo', 'non-unicdoe string')

Diff for: test/strip-trailing-slashes.js

+1
Original file line numberDiff line numberDiff line change
@@ -3,5 +3,6 @@ const stripSlash = require('../lib/strip-trailing-slashes.js')
33
const short = '///a///b///c///'
44
const long = short.repeat(10) + '/'.repeat(1000000)
55

6+
t.equal(stripSlash('no slash'), 'no slash')
67
t.equal(stripSlash(short), '///a///b///c')
78
t.equal(stripSlash(long), short.repeat(9) + '///a///b///c')

0 commit comments

Comments
 (0)