-
Notifications
You must be signed in to change notification settings - Fork 27
perf: Fix AssetService recursion bottleneck and add 10k file circuit breaker #446
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
Open
Ayush4958
wants to merge
2
commits into
StatTag:master
Choose a base branch
from
Ayush4958:imp-prf-for-large-imports
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+51
−7
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -86,12 +86,17 @@ export default class AssetService { | |||
| * @param {string} uri The base URI to recursively scan | ||||
| * @returns An asset object which contains nested assets | ||||
| */ | ||||
| scan(uri) { | ||||
| /** | ||||
| * Internal recursive function to build the asset tree without applying handlers. | ||||
| * This operates extremely quickly (tree-only scan). | ||||
| * @param {string} uri The current URI | ||||
| * @param {object} stats Object to keep track of total files and directories | ||||
| * @returns An asset object | ||||
| */ | ||||
| _buildTree(uri, stats) { | ||||
| // This will throw an error if it can't access the uri | ||||
| fs.accessSync(uri); | ||||
|
|
||||
| // TODO: When we move past file/folder assets, this will need to account for | ||||
| // other types of assets that aren't reachable via the file system. | ||||
| const details = fs.statSync(uri); | ||||
| let result = {}; | ||||
|
|
||||
|
|
@@ -100,9 +105,13 @@ export default class AssetService { | |||
| return result; | ||||
| } | ||||
|
|
||||
| const type = this.assetType(details); | ||||
| if (type === 'file') stats.totalFiles++; | ||||
| else if (type === 'directory') stats.totalDirectories++; | ||||
|
|
||||
| result = { | ||||
| uri, | ||||
| type: this.assetType(details), | ||||
| type, | ||||
| contentTypes: this.assetContentTypes(uri, details), | ||||
| metadata: [], | ||||
| }; | ||||
|
|
@@ -114,12 +123,32 @@ export default class AssetService { | |||
| const files = fs.readdirSync(uri); | ||||
| const children = []; | ||||
| files.forEach(function eachFile(file) { | ||||
| // Skip common large dependency/hidden directories to save massive memory and time | ||||
| if (['node_modules', '.git', '.venv', 'venv', '__pycache__', '.pytest_cache', '.idea'].includes(file)) { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we leverage the list already at: Line 11 in 1ea7b4f
Also, I'm realizing |
||||
| return; // continue in forEach | ||||
| } | ||||
|
|
||||
| const filePath = path.join(uri, file); | ||||
| children.push(self.scan(filePath)); | ||||
| children.push(self._buildTree(filePath, stats)); | ||||
| }); | ||||
|
|
||||
| result.children = children; | ||||
| } | ||||
|
|
||||
| return result; | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Scan a URI for all available assets. This is done recursively for all available assets. | ||||
| * | ||||
| * This will return URIs as absolute paths (not relative). | ||||
| * | ||||
| * @param {string} uri The base URI to recursively scan | ||||
| * @returns An asset object which contains nested assets | ||||
| */ | ||||
| scan(uri) { | ||||
| const stats = { totalFiles: 0, totalDirectories: 0 }; | ||||
| const result = this._buildTree(uri, stats); | ||||
|
|
||||
| if (!this.handlers) { | ||||
| return result; | ||||
|
|
@@ -130,9 +159,24 @@ export default class AssetService { | |||
| } | ||||
|
|
||||
| let assetEntry = result; | ||||
| for (let index = 0; index < this.handlers.length; index++) { | ||||
| assetEntry = this.handlers[index].scan(assetEntry); | ||||
|
|
||||
| // Performance threshold check: if files exceed 10000, skip deep code handler scan | ||||
| // We only apply FileHandler which is fast because it uses already existing fs.stat. | ||||
| // The rest of the handlers are skipped to prevent UI lockup and memory exhaustion. | ||||
| const MAX_DEEP_SCAN_FILES = 10000; | ||||
| if (stats.totalFiles > MAX_DEEP_SCAN_FILES) { | ||||
| console.warn(`[AssetService] Project has ${stats.totalFiles} files, exceeding limit of ${MAX_DEEP_SCAN_FILES}. Skipping deep code handlers.`); | ||||
| for (let index = 0; index < this.handlers.length; index++) { | ||||
| if (this.handlers[index].id && this.handlers[index].id() === 'StatWrap.FileHandler') { | ||||
| assetEntry = this.handlers[index].scan(assetEntry); | ||||
| } | ||||
| } | ||||
| } else { | ||||
| for (let index = 0; index < this.handlers.length; index++) { | ||||
| assetEntry = this.handlers[index].scan(assetEntry); | ||||
| } | ||||
| } | ||||
|
|
||||
| return assetEntry; | ||||
| } | ||||
| } | ||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Minor stylistic thing - please add braces and newlines to separate if/elseif
Sorry, I'm picky about this! :)