src: optimize NormalizeString to cut allocations and copies - #63753
src: optimize NormalizeString to cut allocations and copies#63753whoekage wants to merge 2 commits into
Conversation
|
The changes look good overall. Since this is oriented towards improving performance, could you add some benchmarks here so that we can verify the improvements here? It would also be helpful to have those for any potential future changes. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Rewrite the parent-dir ("..") handling in NormalizeString() to use a
write-position rewind with a "dotdot" floor index, the approach used by
Go's path/filepath.Clean, and build segments with in-place appends
instead of temporary-string concatenation.
Previously each ".." did `res = res.substr(0, idx)`, which allocated a
new string and copied the whole surviving prefix (O(n^2) copy volume on
deep-backtrack paths), plus two find_last_of() scans. Each normal
segment built two or three temporary std::strings, and res was never
reserve()d.
Now ".." rewinds the write position to the previous separator and
resize()s, so there is no allocation, no prefix copy and no
find_last_of call. Segments are written with push_back() and append().
The output is byte for byte identical to the previous implementation.
Signed-off-by: Phail' Sharkaev <sharkaev.ph@gmail.com>
process.permission.has() resolves the reference natively through PathResolve() and NormalizeString() in src/path.cc before the granted tree lookup, and string references are passed through from JS unchanged, so it is a JS-reachable entry point for the native path normalization on all platforms. The new benchmark feeds it paths of different shapes: already normalized, `.` segments with repeated separators, a few `..` segments, many segments without backtracking, and a deep descent followed by an equally deep backtrack. This makes changes to NormalizeString() measurable with benchmark/compare.js. Signed-off-by: Phail' Sharkaev <sharkaev.ph@gmail.com>
78315ce to
6a2778d
Compare
|
@StefanStojanovic added
For reference, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #63753 +/- ##
==========================================
- Coverage 90.19% 90.19% -0.01%
==========================================
Files 771 771
Lines 264628 264883 +255
Branches 50237 50311 +74
==========================================
+ Hits 238683 238899 +216
- Misses 16953 16962 +9
- Partials 8992 9022 +30
🚀 New features to boost your workflow:
|
What this changes
NormalizeString()insrc/path.ccis the lexical path-normalization corebehind
PathResolve(),NormalizeFileURLOrPath()andToNamespacedPath().It collapses
//, removes., resolves.., and strips trailingseparators, and it runs during module and package resolution, the compile
cache,
--permissionfs checks, and (on Windows) on every fs syscall viaToNamespacedPath().This rewrites the
..handling to a write-position rewind with adotdotfloor index, the approach used by Go's
path/filepath.Clean, and switchessegment building to in-place appends.
Before
For every
..segment the old code:res = res.substr(0, idx), which allocated a new string and copied theentire surviving prefix (O(n²) copy volume on deep-backtrack paths);
find_last_of(separator)scans, one to truncate and one torecompute
lastSegmentLength.For every normal segment it built two or three temporary
std::strings(
std::string(separator) + std::string(path.substr(...))), andreswasnever
reserve()d.After
dotdotfloor index marks where backtracking must stop. Resolving..rewinds the write position to the previous separator and calls
res.resize(), so there is no allocation, no prefix copy and nofind_last_of.push_back()andappend(ptr, count), with notemporary strings.
res.reserve(path.size())is called once (the output is always no longerthan the input).
lastSegmentLengthand the "does res already end with.." check aregone, subsumed by
dotdot.The change is 30 insertions and 33 deletions in
src/path.cc.Benchmarks
Measured with an A/B harness that compiles the old and new implementations
into a single binary (clang
-O2, Apple M-series), corroborated by a cctesttiming run. Time per call:
/usr/local/bin/node(typical)a/+ 50x../Heap allocations per call, counted with an
operator newoverride: the50x a/ + 50x ../case drops from 41 to 1, the 200-segment path from 6 to 1,and typical or short paths stay at 0 (small-string optimization).
Correctness
The output is byte for byte identical to the previous implementation. I
verified this with a differential test of 1.2M random inputs (length 0 to 40
over
{'/','\\','.','a','b','c'}, bothallowAboveRootvalues) against afrozen copy of the old implementation, under both POSIX semantics (separator
/) and Windows semantics (IsPathSeparatormatching/and\, withseparators
/and\): 0 mismatches. The existingcctestPathResolvecases pass, and new cases lock the touched behaviors (deep backtrack to root,
the
..floor withallowAboveRoot, and.,//and trailing-separatorcollapse).
Scope
This is a hot helper, but on POSIX it is a small fraction of end-to-end JS
workloads. The main beneficiaries are path-resolution-heavy code and, on
Windows, fs operations, where
ToNamespacedPathruns on every syscall. Thechange is purely a constant-factor and allocation improvement with identical
observable behavior.
Prior art: Go
path/filepath.Clean(lazybuf plus adotdotindex). Arelated allocation optimization in this subsystem that was accepted is #50288.
Checklist
make -j4 test(UNIX) passes for the touched area (cctestPathTest*)