explorer: use map for ExplorerItem children to reduce isEqualOrParent calls #46235
Conversation
You should be 100% sure about that! I would still go with a map (O(1) is much better than O(log n)) - I understand the change is bigger but bigger is better, right ;-) |
|
@jrieken the issue with the map is that I would probably have to keep both the map and the array in parallel, since there are a lot of outisde users of |
|
Yeah, tight coupling... I am sorry for you. The |
|
After discussing with @jrieken and @bpasero we decided to
I have tested out this changes and they look good. Unit tests are also passing and this is covered well I would say. If we decide to merge it in I would create a test plan item so we get good coverage on every platform. @jrieken @bpasero let me know if it is ok by you to merge or even better if you want to code review. |
isidorn
changed the title
| return found; | ||
| }); | ||
| return { exists, child: duplicateChild }; | ||
| duplicateChild = parent.children[isLinux ? name : name.toLowerCase()]; |
There was a problem hiding this comment.
@isidorn this could be a utility method that takes care of the platform specific lowercasing (e.g. stat.hasChild)
| public resource: URI; | ||
| public name: string; | ||
| public mtime: number; | ||
| public etag: string; | ||
| private _isDirectory: boolean; | ||
| private _isSymbolicLink: boolean; | ||
| public children: FileStat[]; | ||
| public parent: FileStat; | ||
| public children: { [name: string]: ExplorerItem }; |
There was a problem hiding this comment.
@isidorn should we use a Map here instead?
| oldLocalChildren.set(localChild.resource, localChild); | ||
| }); | ||
| for (let name in local.children) { | ||
| oldLocalChildren.set(local.children[name].resource, local.children[name]); |
There was a problem hiding this comment.
@isidorn suggest to extract a local variable to prevent duplicate access (var foo = local.children[name])
| @@ -358,20 +368,20 @@ export class NewStatPlaceholder extends FileStat { | |||
| throw new Error('Can\'t perform operations in NewStatPlaceholder.'); | |||
| } | |||
|
|
|||
| public rename(renamedStat: NewStatPlaceholder): void { | |||
| public rename(renamedStat: IFileStat): void { | |||
There was a problem hiding this comment.
@isidorn I think this can go back to NewStatPlaceholder
| throw new Error('Can\'t perform operations in NewStatPlaceholder.'); | ||
| } | ||
|
|
||
| public find(resource: URI): NewStatPlaceholder { | ||
| public find(resource: URI): ExplorerItem { |
There was a problem hiding this comment.
@isidorn I think this can go back to NewStatPlaceholder
| return null; //Unable to find | ||
| } | ||
|
|
||
| public findByPath(path: string, index: number): ExplorerItem { |
| if (resources.isEqual(resource, this.resource, !isLinux /* ignorecase */)) { | ||
| return this; | ||
| if (resource && this.resource.scheme === resource.scheme && this.resource.authority === resource.authority && | ||
| startsWith(resource.path, this.resource.path) |
There was a problem hiding this comment.
@isidorn this checks seems to not work anymore if the path is the same but with different case? With our previous solution this would work. Also, given the code that is implemented in findByPath why do we actually need this extra check, isn't that covered by that other method already?
| for (let i = 0; i < this.children.length; i++) { | ||
| const child = this.children[i]; | ||
| if (this.children) { | ||
| while (index < path.length && path[index] === paths.sep) { |
There was a problem hiding this comment.
@isidorn I understand the code below after some time but this could benefit from some comments what you are trying to achieve here
| for (let childName in stat.children) { | ||
| this.getResolvedDirectories(stat.children[childName], resolvedDirectories); | ||
| } | ||
| stat.getChildrenNames().forEach(name => { |
Fixes #45972
This is my attempt to remove all calls to
isEqualOrParentfrom the explorer.This PR takes advantage of the children array being already alhabetically sorted by names ( I am not 100% sure this is a fair assumption to take, but it is always sorted on my os x).
I decided to take this suggestion from @jrieken as it compared to others seemed to require the least amount of changes. (keeping children in a map would require to change all the method but we can do that as well).
I tried this out and seems to work, also all the explorer tests are passing.
Note that this is not thoroughly tested and just wanted to get some feedback if this approach makes sense thus I created this PR.
Also note that I did not measure performance which I could do in case we decide to take this route.
Let me know what you think
fyi @bpasero