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
Rid of ResolveLinkTarget from IsWindowsApplication #16295
Conversation
We are not taking this for 7.2 GA, we will review this for 7.3 |
@adityapatwardhan @daxian-dbw It is a regression after #16165 so please consider for 7.2. I think the issue block running exe-s from any file system that do not support reparse points (ISO, FAT, Wim(?), ...). |
Well, it's a regression from #16044, not #16165. @adityapatwardhan @TravisEz13 Looking this again, it turns out the regression is from 7.1. The |
// 2. Keep the same behavior as before adopting the 'LinkTarget' and 'ResolveLinkTarget' APIs in .NET 6. | ||
string linkTarget = File.ResolveLinkTarget(fileName, returnFinalTarget: false)?.FullName; | ||
if (linkTarget is not null) | ||
using var stream = new FileStream(fileName, FileMode.Open, FileAccess.Read, FileShare.Read); |
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.
Now that ResolveLinkTarget
is gone do we need to handle unresolvable targets with a better error message than what new FileStream
will raise?
It throws only if something goes wrong when resolving a link. It doesn't throw when a link cannot be resolved, such the AppxExeLink files. |
src/System.Management.Automation/engine/NativeCommandProcessor.cs
Outdated
Show resolved
Hide resolved
699585e
to
94a7c8c
Compare
@TravisEz13 and @adityapatwardhan, I had been in the impression that this PR was to fix the A simpler fix would be to wrap try
{
string linkTarget = File.ResolveLinkTarget(fileName, returnFinalTarget: false)?.FullName;
if (linkTarget is not null)
{
fileName = linkTarget;
}
}
catch
{
} @iSazonov's fix could be better in performance, but it's comparatively a larger change than the above, which may introduce unknown side effects. What about we go with the conservative fix for GA? We can closely review @iSazonov's fix for 7.3. |
The old review comment was off the topic of this PR.
@iSazonov Apologies for my earlier off-topic comments. I thought this was for fixing I submitted the PR #16310 with the more conservative fix. That fix is safer to be included in 7.2 GA at this point. I borrowed your test though, thanks to you and @jborean93 for that! |
I have no objections to get the PR in 7.3 if you see a risk for 7.2. Originally I prepared this as performance fix and I have some more ideas for improving startup scenario but a bug in .Net was fixed too late and PerfView did not work with new traces until then. With .Net we already have ~20% perf win in startup scenario and if MSFT team want we could get another up to ~10%. |
10% speed up in startup is of course what we would want. If that the use JSON .NET instead of NewtonSoft in configuration? |
Yes, the PR is ready and you can measure. .And PSVersion with source generator, PR is ready too. Also I see in traces PSStyle too early and expensive initialization (in static InitialSessionState()), |
94a7c8c
to
e25eaac
Compare
I run benchmarks for both the current As is shown in the results, the
Given this, I don't think we should make changes to the existing code.
CLICK to see Benchmark source code
using System;
using System.IO;
using System.Collections.Generic;
using System.Runtime.InteropServices;
using System.Reflection.PortableExecutable;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using BenchmarkDotNet.Jobs;
namespace benchmark
{
class Program
{
static void Main(string[] args)
{
var summary = BenchmarkRunner.Run<Benchmark_IsWindowsApplication>();
}
}
[SimpleJob(RuntimeMoniker.Net60)]
public class Benchmark_IsWindowsApplication
{
[ParamsSource(nameof(ValuesForPath))]
public string ExePath { get; set; }
public IEnumerable<string> ValuesForPath()
{
yield return @"C:\Program Files\PowerShell\7\pwsh.exe";
yield return @"C:\WINDOWS\system32\notepad.exe";
yield return @"C:\arena\tmp\result.txt";
yield return @"C:\Users\rocky\AppData\Local\Microsoft\WindowsApps\wt.exe";
}
[Benchmark]
public bool RunOld()
{
return IsWindowsApplication_OLD(ExePath);
}
[Benchmark]
public bool RunNew()
{
return IsWindowsApplication_NEW(ExePath);
}
internal static bool IsWindowsApplication_NEW(string fileName)
{
try
{
using var stream = new FileStream(fileName, FileMode.Open, FileAccess.Read, FileShare.Read);
PEHeader peheader = (new PEHeaders(stream)).PEHeader;
return peheader is not null && peheader.Subsystem == Subsystem.WindowsGui;
}
catch
{
return false;
}
}
internal static bool IsWindowsApplication_OLD(string fileName)
{
SHFILEINFO shinfo = new SHFILEINFO();
IntPtr type = SHGetFileInfo(fileName, 0, ref shinfo, (uint)Marshal.SizeOf(shinfo), SHGFI_EXETYPE);
switch ((int)type)
{
case 0x0:
// 0x0 = not an exe
return false;
case 0x5a4d:
// 0x5a4d - DOS .exe or .com file
return false;
case 0x4550:
// 0x4550 - windows console app or bat file
return false;
default:
// anything else - is a windows program...
return true;
}
}
[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
private struct SHFILEINFO
{
public IntPtr hIcon;
public int iIcon;
public uint dwAttributes;
[MarshalAs(UnmanagedType.ByValTStr, SizeConst = 260)]
public string szDisplayName;
[MarshalAs(UnmanagedType.ByValTStr, SizeConst = 80)]
public string szTypeName;
}
private const uint SHGFI_EXETYPE = 0x000002000; // flag used to ask to return exe type
[DllImport("shell32.dll", CharSet = CharSet.Unicode)]
private static extern IntPtr SHGetFileInfo(string pszPath, uint dwFileAttributes,
ref SHFILEINFO psfi, uint cbSizeFileInfo, uint uFlags);
}
} |
What is the reason you are measuring the opposite scenario? This optimization is just for exe files, which is the most common use. Or do you think users prefer to run wt.exe in a loop 100 times per second? :-) Also Windows team doesn't publish APIs for appx. So we can not detect whether it is a console or GUI and so on. Before we could do something useful for appx MSFT must introduce public API for this. |
I don't see this in PEHeader sources - it seek a offset only. |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Why do you exclude File.ResolveLinkTarget from IsWindowsApplication_OLD? Sly! :-) |
Yeah, I missed that line of code :) But it also occurred to me that we don't need that line of code today given that it doesn't work for Windows Store apps ( The whole reason that
The
Yeah, both approaches return
I didn't look into the |
So I understand from your comment that negotiations with Windows team were fruitless and we will not get public appx API. In the case, yes, we should simply remove |
From the last conversation with the Windows Store team about this, they said it was by-design to not document that API, because they might change how the mapping goes behind the scene. I don't think anyone from our team has started the conversation again after we learnt about this regression in Can you maybe update your PR to remove |
Too long path :-) Will do new PR. |
Yeah, a new PR is better, making the purpose clearer too. I will close this PR then. |
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.
LGTM
PR Summary
Fix #16293 I tested manually and confirm the fix works for mounted ISOs.New test is based on ISO with FAT file system (no reparse point support).I prepared the PR as performance improvement a week before #16293 :-)
PR Context
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).