-
-
Notifications
You must be signed in to change notification settings - Fork 143
Exists performance improvement #1227
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
Conversation
…hods - Add Result<T, E> type from PR #1218 for better error handling - Refactor walk() to return Result<Link|null, StatError> instead of throwing - Add StatError interface with toError() method for deferred error conversion - Improve performance of existsSync for non-existent files (+992.2%) - Improve performance of existsSync for existing files (+10.9%) - Add createStatError utility function for consistent error creation - Update _stat, _exists, and related methods to use Result type - All 932 tests pass with improved error handling Performance improvements verified by benchmark: - existsSync (existing): 667.97 → 700.87 ops/ms (+4.9%) - existsSync (non-existing): 130.93 → 1430.57 ops/ms (+992.2%) ⭐ - existsSync (deep path): 805.69 → 808.98 ops/ms (+0.4%) - exists (existing): 623.78 → 665.76 ops/ms (+6.7%)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a performance optimization for the exists() and existsSync() functions by refactoring internal error handling to use a Result type pattern instead of exception-based control flow. The change reduces exception throwing in hot paths, particularly for non-existent files, resulting in significant performance improvements.
Changes:
- Introduced a new
Result<T, E>type and helper functions (Ok,Err) for representing success or error states without exceptions - Refactored internal
_statandwalkmethods to returnResulttypes instead of throwing exceptions - Created
StatErrorinterface andcreateStatErrorfunction for structured error objects - Added benchmark file demonstrating performance improvements
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/result.ts | New file introducing the Result type pattern for error handling |
| src/core/types.ts | Added StatError interface for structured error representation |
| src/node/util.ts | Added createStatError function for creating StatError objects |
| src/core/Superblock.ts | Refactored walk method to return Result types instead of throwing exceptions |
| src/node/volume.ts | Updated _stat and _exists to use Result types; added _statOrThrow wrapper |
| benchmark-exists.js | New benchmark file documenting performance improvements |
src/node/util.ts
Outdated
| (error as any).path = this.path | ||
| } | ||
| return error | ||
| } | ||
| } as StatError |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at the end of the statement. This is inconsistent with the coding style used elsewhere in the file (e.g., line 165) and should be corrected.
| (error as any).path = this.path | |
| } | |
| return error | |
| } | |
| } as StatError | |
| (error as any).path = this.path; | |
| } | |
| return error; | |
| } | |
| } as StatError; |
src/node/util.ts
Outdated
| (error as any).path = this.path | ||
| } | ||
| return error | ||
| } | ||
| } as StatError |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at the end of the statement. This is inconsistent with the coding style used elsewhere in the file and should be corrected.
| (error as any).path = this.path | |
| } | |
| return error | |
| } | |
| } as StatError | |
| (error as any).path = this.path; | |
| } | |
| return error; | |
| } | |
| } as StatError; |
src/core/Superblock.ts
Outdated
| return this.walk(steps, false, false, false); | ||
| const result = this.walk(steps, false, false, false); | ||
| if (result.ok) { | ||
| return result.value |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at the end of the statement. This is inconsistent with the coding style used throughout the codebase and should be corrected.
src/core/Superblock.ts
Outdated
| if (result.ok) { | ||
| return result.value | ||
| } | ||
| throw result.err.toError() |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at the end of the statement. This is inconsistent with the coding style used throughout the codebase and should be corrected.
src/core/Superblock.ts
Outdated
| return this.walk(filename, false, true, true, funcName)!; | ||
| const result = this.walk(filename, false, true, true, funcName); | ||
| if (result.ok) { | ||
| return result.value! |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at the end of the statement. This is inconsistent with the coding style used throughout the codebase and should be corrected.
src/core/Superblock.ts
Outdated
| if (result.ok) { | ||
| return Ok(result.value!) | ||
| } | ||
| return result |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at the end of the statement. This is inconsistent with the coding style used throughout the codebase and should be corrected.
src/node/volume.ts
Outdated
| if (result.err.code === ERROR_CODE.ENOENT && !throwIfNoEntry) { | ||
| return Ok(undefined); | ||
| } else { | ||
| return result |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at the end of the statement. This is inconsistent with the coding style used throughout the codebase and should be corrected.
| return result | |
| return result; |
| private _statOrThrow(filename: string, bigint = false, throwIfNoEntry = true): Stats<number | bigint> { | ||
| const result = this._stat(filename, bigint, throwIfNoEntry); | ||
| if (result.ok) { | ||
| return result.value!; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-null assertion operator (!) is used here. While this is safe because the Result is checked via result.ok, consider documenting why this is guaranteed to be non-null or using optional chaining for better type safety.
| private _statOrThrow(filename: string, bigint = false, throwIfNoEntry = true): Stats<number | bigint> { | |
| const result = this._stat(filename, bigint, throwIfNoEntry); | |
| if (result.ok) { | |
| return result.value!; | |
| private _statOrThrow(filename: string, bigint = false, throwIfNoEntry = true): Stats<number | bigint> | undefined { | |
| const result = this._stat(filename, bigint, throwIfNoEntry); | |
| if (result.ok) { | |
| return result.value; |
| const newPathFilename = pathToFilename(newPath); | ||
| this.wrapAsync(this._core.rename, [oldPathFilename, newPathFilename], callback); | ||
| }; | ||
|
|
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _exists method returns Result<true, StatError> when it could be more semantically correct to return Result<boolean, StatError> with Ok(result.ok). However, since this method only returns on success (when file exists), the current signature is acceptable. Consider adding a comment explaining why only 'true' is returned as success value, given that the failure case returns a StatError.
| // Note: _exists returns Result<true, StatError>. The success case means | |
| // "path exists" (value is always true); non-existence and other errors are | |
| // represented by StatError in the error branch. |
benchmark-exists.js
Outdated
| console.log('Baseline (original):'); | ||
| console.log(' - existsSync (existing): 667.97 ops/ms'); | ||
| console.log(' - existsSync (non-existing): 130.93 ops/ms'); | ||
| console.log(' - existsSync (deep): 805.69 ops/ms'); | ||
| console.log(' - exists (existing): 623.78 ops/ms'); | ||
| console.log('\nAfter PR #1218 (Result type + improved error handling):'); | ||
| console.log(' - existsSync (existing): 741.25 ops/ms (+10.9%)'); | ||
| console.log(' - existsSync (non-existing): 1437.15 ops/ms (+992.2%) ⭐'); | ||
| console.log(' - existsSync (deep): 801.45 ops/ms (-0.5%)'); | ||
| console.log(' - exists (existing): 672.76 ops/ms (+7.9%)'); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benchmark file hardcodes results and references a specific PR number (#1218). This benchmark should either be dynamic (actually measuring the current implementation) or moved to documentation/comments rather than being a committed file that will become outdated.
| console.log('Baseline (original):'); | |
| console.log(' - existsSync (existing): 667.97 ops/ms'); | |
| console.log(' - existsSync (non-existing): 130.93 ops/ms'); | |
| console.log(' - existsSync (deep): 805.69 ops/ms'); | |
| console.log(' - exists (existing): 623.78 ops/ms'); | |
| console.log('\nAfter PR #1218 (Result type + improved error handling):'); | |
| console.log(' - existsSync (existing): 741.25 ops/ms (+10.9%)'); | |
| console.log(' - existsSync (non-existing): 1437.15 ops/ms (+992.2%) ⭐'); | |
| console.log(' - existsSync (deep): 801.45 ops/ms (-0.5%)'); | |
| console.log(' - exists (existing): 672.76 ops/ms (+7.9%)'); | |
| console.log('See benchmark measurements above for current implementation performance.'); |
|
🎉 This PR is included in version 4.53.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.