remove dead code from nativemethods

[kasperk81]

followup on #9223

[JanKrivanek]

Hello @kasperk81 - thank you for your contribution! MSBuild team for sure appraciates contributions. Upfront heads up via bug or other form of discussion is preferred (plus an explicit confirmation of wanting to fix).

More details: https://github.com/dotnet/msbuild/blob/main/documentation/wiki/Contributing-Code.md

Each PR should ideally have motivation and goal in the description (links are big advantage, but important context should be part of the PR).

By no means would I want to kill the interest and flow :-) But it should be just little extra work that will save us considerable time.

---

In this specific PR - it mentiones it's a followup of the other PR that uses IO.Redist on NetFx, while here only single implementation is used - it's bit confusing to me. It looks great as code cleanup. Though as mentioned followup I'd expect the usage of IO.Redist or justification why not using it

[kasperk81]

while here only single implementation is used

the change in WindowsFileSystem.cs is following the other pr, MSBuildTaskHostFileSystem.cs is only built on netfx so it didn't required netcore tfm branch and that's the only difference there.

[JanKrivanek]

while here only single implementation is used

the change in WindowsFileSystem.cs is following the other pr, MSBuildTaskHostFileSystem.cs is only built on netfx so it didn't required netcore tfm branch and that's the only difference there.

Is it just code removal though? The previous implementation of MSBuildTaskHostFileSystem.FileOrDirectoryExists ended up in FileOrDirectoryExistsWindows

msbuild/src/Framework/NativeMethods.cs

Lines 1865 to 1869 in d8d42ef

	internal static bool FileOrDirectoryExistsWindows(string path)
	{
	WIN32_FILE_ATTRIBUTE_DATA data = new WIN32_FILE_ATTRIBUTE_DATA();
	return GetFileAttributesEx(path, 0, ref data);
	}

Now it calls System.IO.[File|Directory]

It looks like a change in a good direction. It however doesn't look like dead code removal. Let's please add a motivation + reaso ning into PR description so that we can proceed with reviewing.

[maridematte]

I agree with Jan here, please add more about the context as it is a bit hard to understand where these changes are coming from.

[kasperk81]

@JanKrivanek seems like we are reading too much into semantics in a simple code cleanup moving stuff from native to managed. not the first pr of its kind in this repo. p/invoking win32 api for file.exists check looks to be some legacy from 90s which is no longer needed as green ci status is showing up here

[rainersigwald]

p/invoking win32 api for file.exists check looks to be some legacy from 90s which is no longer needed as green ci status is showing up here

How did you evaluate the perf impact of this? Historically we've used a variety of P/Invokes on Windows because they could be tuned to be faster than the BCL mechanisms. Because of that I'm hesitant to remove any without some fairly detailed perf analysis work.

[kasperk81]

here is the benchmark https://gist.github.com/kasperk81/6e0c449a88ef95df8b30f1d9d5db4771

on windows vm

| Method                               | Mean     | Error     | StdDev    |
|------------------------------------- |---------:|----------:|----------:|
| FileExistsUsingPInvoke               | 1.760 ms | 0.0339 ms | 0.0487 ms |
| FileExistsUsingPathExists            | 1.712 ms | 0.0336 ms | 0.0345 ms |
| FileExistsUsingFileOrDirectoryExists | 1.751 ms | 0.0344 ms | 0.0422 ms |

no difference, if fraction of nano-seconds count then the change is better but i'd rule it out as noise. framework seems to be doing an ok job