-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add a simple queue implementation with better performance than Array.shift
#49623
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -504,18 +504,18 @@ namespace ts.server { | |
// If `getResultsForPosition` returns results for a project, they go in here | ||
const resultsMap = new Map<Project, readonly TResult[]>(); | ||
|
||
const queue: ProjectAndLocation[] = []; | ||
const queue = createQueue<ProjectAndLocation>(); | ||
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. Even here, does order matter if not we could get away with having to move array elements right? 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. I believe order does matter, though that doesn't necessarily mean we couldn't accomplish the same thing with a stack. 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. why does order matter ? in all find all references etc when aggregating results between project, the order shouldnt matter right ? Yes our baselines may change but from end to end result perspective order shouldnt matter ? 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. I believe the search order affects the value of |
||
|
||
// In order to get accurate isDefinition values for `defaultProject`, | ||
// we need to ensure that it is searched from `initialLocation`. | ||
// The easiest way to do this is to search it first. | ||
queue.push({ project: defaultProject, location: initialLocation }); | ||
queue.enqueue({ project: defaultProject, location: initialLocation }); | ||
|
||
// This will queue `defaultProject` a second time, but it will be dropped | ||
// as a dup when it is dequeued. | ||
forEachProjectInProjects(projects, initialLocation.fileName, (project, path) => { | ||
const location = { fileName: path!, pos: initialLocation.pos }; | ||
queue.push({ project, location }); | ||
queue.enqueue({ project, location }); | ||
}); | ||
|
||
const projectService = defaultProject.projectService; | ||
|
@@ -536,25 +536,13 @@ namespace ts.server { | |
const searchedProjectKeys = new Set<string>(); | ||
|
||
onCancellation: | ||
while (queue.length) { | ||
while (queue.length) { | ||
while (!queue.isEmpty()) { | ||
while (!queue.isEmpty()) { | ||
if (cancellationToken.isCancellationRequested()) break onCancellation; | ||
|
||
let skipCount = 0; | ||
for (; skipCount < queue.length && resultsMap.has(queue[skipCount].project); skipCount++); | ||
|
||
if (skipCount === queue.length) { | ||
queue.length = 0; | ||
break; | ||
} | ||
|
||
if (skipCount > 0) { | ||
queue.splice(0, skipCount); | ||
} | ||
|
||
// NB: we may still skip if it's a project reference redirect | ||
const { project, location } = queue.shift()!; | ||
const { project, location } = queue.dequeue(); | ||
|
||
if (resultsMap.has(project)) continue; | ||
if (isLocationProjectReferenceRedirect(project, location)) continue; | ||
|
||
const projectResults = searchPosition(project, location); | ||
|
@@ -574,7 +562,7 @@ namespace ts.server { | |
if (resultsMap.has(project)) return; // Can loop forever without this (enqueue here, dequeue above, repeat) | ||
const location = mapDefinitionInProject(defaultDefinition, project, getGeneratedDefinition, getSourceDefinition); | ||
if (location) { | ||
queue.push({ project, location }); | ||
queue.enqueue({ project, location }); | ||
} | ||
}); | ||
} | ||
|
@@ -604,7 +592,7 @@ namespace ts.server { | |
|
||
for (const project of originalScriptInfo.containingProjects) { | ||
if (!project.isOrphan() && !resultsMap.has(project)) { // Optimization: don't enqueue if will be discarded | ||
queue.push({ project, location: originalLocation }); | ||
queue.enqueue({ project, location: originalLocation }); | ||
} | ||
} | ||
|
||
|
@@ -613,7 +601,7 @@ namespace ts.server { | |
symlinkedProjectsMap.forEach((symlinkedProjects, symlinkedPath) => { | ||
for (const symlinkedProject of symlinkedProjects) { | ||
if (!symlinkedProject.isOrphan() && !resultsMap.has(symlinkedProject)) { // Optimization: don't enqueue if will be discarded | ||
queue.push({ project: symlinkedProject, location: { fileName: symlinkedPath as string, pos: originalLocation.pos } }); | ||
queue.enqueue({ project: symlinkedProject, location: { fileName: symlinkedPath as string, pos: originalLocation.pos } }); | ||
} | ||
} | ||
}); | ||
|
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.
To me this part of the code seems like that order wouldnt matter so always using last element so i wonder if always using last element and keeping tail thats used to add more items and sets array.length to compact array once in a while might be better.
Thoughts?
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.
I didn't read it carefully when I made the change - I just checked that it would behave the same after. Reading it now, it looks like switching from a queue to a stack would change a BFS to a DFS, which may or may not be desirable.