Change calls from string.IndexOf to string.Contains (CoreFX) #32249
Conversation
grant-d
changed the title
grant-d
changed the title
| @@ -174,7 +174,7 @@ public static void EncodeTags(int tags, ref int pos, byte[] metadata) | |||
|
|
|||
| public static void CheckName(string name) | |||
| { | |||
| if (name != null && 0 <= name.IndexOf('\0')) | |||
There was a problem hiding this comment.
FYI, you don't need to make the changes to src/Common/src/CoreLib/* again here.
The changes from dotnet/coreclr#19874 will get mirrored to this repository automatically.
There was a problem hiding this comment.
OK, didn't realize that. Thanks.
src/Common/src/CoreLib/System/Diagnostics/Tracing/TraceLogging/Statics.cs
src/Common/src/CoreLib/System/Globalization/CalendarData.Unix.cs
src/Common/src/CoreLib/System/Globalization/CultureData.cs
src/Common/src/CoreLib/System/IO/Path.Unix.cs
src/Common/src/CoreLib/System/IO/Path.Windows.cs
src/Common/src/CoreLib/System/IO/PathHelper.Windows.cs
|
@ahsonkhan please can you let me know what |
|
Also. Would this build typically succeed now that the reference assembly code has been checked in (the other PR you merged today). Or do I need to wait for an SDK push or the like |
It's dotnet-bot test this please ;) You can also use @dotnet-bot help |
|
@dotnet-bot help |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
|
@dotnet-bot test this please |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
I believe it should succeed now. Let's find out :) |
Ha! Serves me right, where's my manners |
|
@dotnet-bot test this please |
|
Hmm... I just realized string.Contains(char) is a .NET Core 2.1+ specific API which is why you are seeing errors around overload matching not working as expected. It is looking for the API that accepts a char but can only find the one that takes string. For certain libraries (that are cross-compiled for different runtimes), you would either need to use #ifdef OR continue to use IndexOf. For instance: For these specific failures (System.Config/Microsoft.CSharp/etc.), I would keep using IndexOf to keep things simple (unless they already have #ifdefs in other places within the source file). FYI, running the CI repeatedly is unnecessary since the CI is fairly reliable and failing legs imply something is wrong that needs to fixed. It is rare that we have some sporadic issue that gets fixed by re-running a specific leg (although that has happened from time to time). |
|
Thanks, I will start working on those. I appreciate the help & advice; dupe CI was unintentional, I didn't notice you'd queued a build until after the fact. |
Fixed per your advice. I have added a comment to them to act as a sentinel should we try to fix (or find) them again in the future. if (name.IndexOf('/') >= 0) // string.Contains(char) is .NetCore2.1+ specific
return null; |
FYI: https://apisof.net/catalog/System.String.Contains(Char) You will likely uncover several other places that won't have access to the overload (particularly for tests that are targeting netstandard).
Hopefully these are it, but I would make sure locally first. |
Thanks, that helped. Submitted reverts, we'll see if it passes this time around. |
|
@dotnet-bot test Packaging All Configurations x64 Debug Build |
|
@ahsonkhan when you have a chance, please let me know how to troubleshoot the failed test. It doesn't seem to show any errors in the console? |
|
Ignore - it was hidden in plain sight, |
| @@ -18,6 +18,7 @@ public void IdnDnsSafeHost_IdnOffWithBuiltInScheme_Success() | |||
|
|
|||
| string dns = test.DnsSafeHost; | |||
|
|
|||
| // string.Contains(char) is .NetCore2.1+ specific | |||
There was a problem hiding this comment.
Comments seem even less useful for tests where we don't care about perf to this level anyway.
There was a problem hiding this comment.
See previous response. Do you want me to revert the comments in tests/asserts?
There was a problem hiding this comment.
Reverted all comments in test/assert. Let me know if you want me to revert all comments
There was a problem hiding this comment.
@ahsonkhan thoughts? The only reason to not revert them all is if they have using System.Linq; then I believe that Contains would compile to a slow Linq version.
There was a problem hiding this comment.
then I believe that Contains would compile to a slow Linq version.
Imo, that's a good enough reason to keep the comment.
That Linq API is quite slow, especially due to boxing:
| Method | IsDataCompact | TestCase | Mean | Error | StdDev | Gen 0 | Allocated |
|---|---|---|---|---|---|---|---|
| StringContains | True | HelloWorld | 2.453 ns | 0.3580 ns | 0.0930 ns | - | 0 B |
| LinqContains | True | HelloWorld | 31.012 ns | 2.0902 ns | 0.5429 ns | 0.0076 | 32 B |
Let's make sure we aren't accidentally calling the Linq.Contains API in hot paths.
There was a problem hiding this comment.
In my cursory look, I was only able to find two possible candidates where we could use IndexOf instead (and avoid using Linq.Contains:
The ones in System.Net.WebHeaderCollection are fine since we target netcoreapp for that project:
There was a problem hiding this comment.
There is another perf comparison here https://github.com/dotnet/corefx/issues/25094 where I originally added the overload. I saw 20x
There was a problem hiding this comment.
20x
Wow. I still have a task on my backlog to see what the per improvement is with this change. I will get to it after the units. (I doubt it will be as good as that)
There was a problem hiding this comment.
two possible candidates
OK, I will try update those too.
BTW, I hadn't though to look for existing callsites using the pattern .Contains("<single char>"). Your SNIProxy note made me realize that and so I have found a few more places I can change, added them to the units PR
|
The following files have Is there anything you want me to do about these - revert changes or otherwise? |
|
@ahsonkhan what is special about the |
grant-d
changed the title
src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs
Show resolved
Hide resolved
|
Thanks @grant-d |
|
No problem. Thanks for the help & coaching |
|
If you'd like something else, we have plenty labeled up for grabs - varying levels of accessibility |
|
Thanks, that's where I grabbed this one from. I am just adding units for this (see #32293) and then I am working on the following which was (originally) also from that query: |
|
Note that the following files have
|
|
I assume they actually need that |
|
Yes, I assume so. So I have undone all changes to those 2 files (Soap/XmlReflectionImporter). |
…corefx#32249) * Update call sites * More call sites * Another callsite * More call sites * CR fixes - revert src/Common/src/CoreLib/* * Spurious fix * Revert string.Contains where platform may not be NetCore21 * nit * More reverts * More reverts * More reverts * Revert * Remove comments * Revert Commit migrated from dotnet/corefx@6ffca61
Containsis optimized overIndexOfSpan<T>.ContainsSpan<T>.Contains