resources: ignore trailing slash when comparing

[isidorn]

fixes #36946

This time did it on resources. Makes more sense here as you said

[bpasero]

@isidorn do we know at which point do resources enter our system that have a trailing slash? I would think that the majority of users of these 2 methods will never have a trailing slash, is that a fair statement?

[isidorn]

@bpasero yes I believe that is a fair statement.
The trailing slash enters the system from the remote fileSystemProvider, an example of that can be found here #36946
Also @jrieken can provide more details since he tried the whole example out

[bpasero]

Imho we should do the normalization at that point if possible. So far we never had trailing slashes ending up in our system because we realpath typically before trusting a workspace folder for example.

[jrieken]

Yeah, I would say wherever we accept and open URIs or paths this can happen and I believe that's OK to happen because the folder /foo can be called /foo or /foo/ or /foo/./ or /foo/../foo/.

What I do think is a little naive and wasteful is the creation new strings all the time, you could embed the logic simply inside the isEqualOrParent function by accepting trailing slashes.

[bpasero]

What I do think is a little naive and wasteful is the creation new strings all the time

Related to that I noticed that ltrim and rtrim seem to return a substring even if the needle is not found in the haystack. I would think those methods could return quickly the haystack if the indexOf is -1.

[jrieken]

For once, we should use trim, trimLeft, and trimRight that come with the browser and use the existing ones as fallback only.

I would think those methods could return quickly the haystack if the indexOf is -1.

I believe they do... While indexOf === 0 and when it's not found it will do a substring of 0 which should be a noop.

[isidorn]

@jrieken I thought the browser string.trim() removes whitespace and not custom characters? Also I can not find trimLeft or trimRight
If the ltrim and rtrim do not return a new string then I guess this implementation is not bad since only in the corner case I will create a new string.
Though if you prefer I can look at exact string indeces to also cover this case to not create an additional string.

[isidorn]

What is needed for this PR so we proceed?
@jrieken would you like me to look at indeces inside isEqualOrParent. Downside of this approach is that it will also effect the paths library not only resources. Also it seems that I am only creating new strings in a corner case
@bpasero do you want me to do some measuring by comparing random paths with and without the pr?

[bpasero]

@isidorn I would just add a check to see if path[path.length] === '/' and only then do the trimming. That should still be fast and avoids any string creation.

[jrieken]

would you like me to look at indeces inside isEqualOrParent. Downside of this approach is that it will also effect the paths library not only resources.

@isidorn I think that's fair and everyone would benefit from it. With 06ba86f#diff-6483307f755ba6e74fda5c954b3d08f2 @bpasero deleted the tests and I would restore them.

Also the way how the function is implemented seems to have changed overtime, esp. that the inputs aren't normalised anymore can be a problem. Think of paths.isEqualOrParent('foo/bar/test.ts', 'foo/./bar') and related cases. Unless it changed path#normalize is 'save' to use because it makes sure to quickly return when paths are already normal. Then the only thing that's left to check is the common prefix length (case sensitive or not) and to check that the remainders are just path separators.

[jrieken]

Also I can not find trimLeft or trimRight

fyi - That's the problem of our lib.d.ts and the endless IE11 story. The implementation tho could check that and export String.trimLeft instead of our custom thing. We do similar things with dom.addClass. However, tweaking the strings-util shouldn't be within the scope of this PR

[bpasero]

As far as I remember, path.normalize() showed up as the top perf hit when running isEqualOrParent or isEqual with many thousand elements (at the time I checked - not sure if meanwhile this method changed). It is true that we no longer support the equals check for non-normalized paths, however so far this has not been an issue likely because our paths are already normalized when entering our system.

Bottom line: isEqual and isEqualOrParent currently do not work with:

• paths that have trailing slashes (even node.js path.normalize() does not seem to remove them)
• paths that have relative segments (../ or ./) within

[jrieken]

Both are called like crazy, one startup in a normal workspace (showing a full explorer), I count 1180 calls to isParentOrEqual and 813 calls to normalize (with 86 non-normal paths). Unsure why that is sooo many.

Since a while (around two years) the normalize-function is optimised for the fact that most paths are normal, on aforementioned startup it costs 2.6ms which you are likely to triple with a correct implementation of isEqualsOrParent (which is maybe OK?)

So, I would recommend the following. Leave paths#isEqualOrParent as incorrect as it is today and stop calling it. Move base/common/resources.ts into a layer which allows the usage of node-api, then use path.posix-functions (e.g path.posix.normalize) and use uri.path only. Use ignoreCase only for file-schemes

[jrieken]

I have filed #45971 and #45972 and I now think that we should make isEqualOrParent correct and we should revisit the callers that are being responsible for ~90% of the calls

[bpasero]

After discussion with @jrieken a pre-req for making isEqual / isEqualOrParent more correct (by using normalize) is to remove this method from hot-loops such as #45971, #46103 and #45972

[isidorn]

We have removed it from the most common hot-loop in explorer. However there are still some usages captured in this item #46549

[bpasero]

...and #46103

[isidorn]

Let's close in favor of #48682