We are not taking this for 7.2 GA, we will review this for 7.3

Aditya Patwardhan (@adityapatwardhan) Dongbo Wang (@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(?), ...).

It is a regression after #16165 so please consider for 7.2.

Well, it's a regression from #16044, not #16165.

Aditya Patwardhan (@adityapatwardhan) Travis Plunk (@TravisEz13) Looking this again, it turns out the regression is from 7.1. The AppExeCLinks support was originally added in 7.1. The original IsWindowsApplication change was done in #13481 in 7.1 to help determine if a Store application is GUI or CUI executable, based on the AppExeCLinks support. Now that the AppExeCLinks support is reverted (by design), the check on Store application is broken. So we should consider this for 7.2 GA, or consider backport it in a servicing release for 7.2 later on.

Well, it's a regression from #16044, not #16165.

In #16165 we replaced WinInternalGetTarget() (no exception) with IO.File.ResolveLinkTarget() (now throw).

In #16165 we replaced WinInternalGetTarget() (no exception) with IO.File.ResolveLinkTarget() (now throw).

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.

Travis Plunk (@TravisEz13) and Aditya Patwardhan (@adityapatwardhan), I had been in the impression that this PR was to fix the AppxExeLink regression, but I was wrong. It's to fix a regression introduced by #16165 in 7.2.0-rc.1. Please review the PR and see if this should be taken for 7.2 GA.

A simpler fix would be to wrap File.ResolveLinkTarget with a try/catch. demonstrated as follows:

try
{
    string linkTarget = File.ResolveLinkTarget(fileName, returnFinalTarget: false)?.FullName;
    if (linkTarget is not null)
    {
        fileName = linkTarget;
    }
}
catch
{
}

Ilya (@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 Ilya (@iSazonov)'s fix for 7.3.

Ilya (@iSazonov) Apologies for my earlier off-topic comments. I thought this was for fixing AppxExeLink regression somehow by mistake 😦

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 Jordan Borean (@jborean93) for that!

What about we go with the conservative fix for GA? We can closely review Ilya (@iSazonov)'s fix for 7.3.

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%.

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?

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()), but no PR have I the PR is ready too.
There is System.Diagnostics.Tracing.EventSource expensive initialization. It seems .Net team has an issue for this but we could do something too although it is not trivial and it may require effort.

I run benchmarks for both the current SHGetFileInfo and the PEHeader approaches, and the result shows that the PEHeader approach is not as performant as the existing SHGetFileInfo approach.

As is shown in the results, the RunOld is always faster than RunNew. RunOld uses SHGetFileInfo while RunNew uses PEHeader. Especially for wt.exe (store app) and result.txt (file with shell-associated app), it's way slower using the PEHeader approach for them. This is because

• for store app, an exception is thrown to get the result when using PEHeader;
• for files, the whole file will be read into memory when using PEHeader.

Given this, I don't think we should make changes to the existing code.

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.19043.1288 (21H1/May2021Update)
Intel Core i7-1065G7 CPU 1.30GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK=6.0.100-rc.2.21505.57
  [Host]   : .NET 6.0.0 (6.0.21.48005), X64 RyuJIT
  .NET 6.0 : .NET 6.0.0 (6.0.21.48005), X64 RyuJIT

Job=.NET 6.0  Runtime=.NET 6.0

| Method |              ExePath |           Mean |        Error |       StdDev |         Median |
|------- |--------------------- |---------------:|-------------:|-------------:|---------------:|
| RunOld | C:\Pr(...)h.exe [38] |    51,141.1 ns |    609.53 ns |    570.16 ns |    51,159.3 ns |
| RunNew | C:\Pr(...)h.exe [38] |    54,983.0 ns |    968.27 ns |    905.72 ns |    55,131.4 ns |
| RunOld | C:\Us(...)t.exe [57] |    18,302.6 ns |    386.74 ns |  1,128.13 ns |    18,002.8 ns |
| RunNew | C:\Us(...)t.exe [57] |    35,779.6 ns |    520.25 ns |    486.64 ns |    35,571.9 ns |
| RunOld | C:\WI(...)d.exe [31] |    60,472.8 ns |    954.44 ns |    846.08 ns |    60,254.5 ns |
| RunNew | C:\WI(...)d.exe [31] |    64,938.8 ns |  1,176.96 ns |  1,043.34 ns |    64,702.2 ns |
| RunOld | C:\ar(...)t.txt [23] |       179.8 ns |      2.97 ns |      2.48 ns |       179.5 ns |
| RunNew | C:\ar(...)t.txt [23] | 1,388,967.6 ns | 25,999.91 ns | 43,440.01 ns | 1,388,259.2 ns |

** C:\Pr(...)h.exe -- C:\Program Files\PowerShell\7\pwsh.exe  (console app)
** C:\Us(...)t.exe -- C:\Users\rocky\AppData\Local\Microsoft\WindowsApps\wt.exe (store app)
** C:\WI(...)d.exe -- C:\WINDOWS\system32\notepad.exe (gui app)
** C:\ar(...)t.txt -- C:\arena\tmp\result.txt (file that has shell-associated app)

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.

for files, the whole file will be read into memory when using PEHeader.

I don't see this in PEHeader sources - it seek a offset only.

This PR has 43 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Label      : Extra Small
Size       : +6 -37
Percentile : 17.2%

Total files changed: 1

Change summary by file extension:
.cs : +6 -37

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

• Fast and predictable releases to production:
  • Optimal size changes are more likely to be reviewed faster with fewer
    iterations.
  • Similarity in low PR complexity drives similar review times.
• Review quality is likely higher as complexity is lower:
  • Bugs are more likely to be detected.
  • Code inconsistencies are more likely to be detetcted.
• Knowledge sharing is improved within the participants:
  • Small portions can be assimilated better.
• Better engineering practices are exercised:
  • Solving big problems by dividing them in well contained, smaller problems.
  • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

• Use the PullRequestQuantifier to quantify your PR accurately
  • Create a context profile for your repo using the context generator
  • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
  • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
  • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
• Change your engineering behaviors
  • For PRs that fall outside of the desired spectrum, review the details and check if:
    • Your PR could be split in smaller, self-contained PRs instead
    • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

• One line was added: +1 -0
• One line was deleted: +0 -1
• One line was modified: +1 -1 (git diff doesn't know about modified, it will
  interpret that line like one addition plus one deletion)
• Change percentiles: Change characteristics (addition, deletion, modification)
  of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

Why do you exclude File.ResolveLinkTarget from IsWindowsApplication_OLD? Sly! :-)

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 (AppExeCLinks).

The whole reason that InternalSymbolicLinkLinkCodeMethods.WinInternalGetTarget(fileName) was originally added here was to fix for the Windows Store apps. You submitted the PR #13481, remember? :)

WinInternalGetTarget was replaced by File.ResolveLinkTarget when deprecating WinInternalGetTarget, but since there is no documented API to retrieve the actual target of a Windows Store app, there is no point to keep File.ResolveLinkTarget anymore. We can totally remove that for now, until the regarding API gets documented.

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? :-)

The PEHeader approach is generally slower than SHGetFileInfo, not just wt.exe. Directly executing a document to open it with the shell-associated application is also quite common, like an excel, an .txt file, or a word file.

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.

Yeah, both approaches return false for Windows Store app, but the PEHeader one requires exception handling for that, so it's way slower. Yeah, this is not a common scenario, but like I said, the PEHeader is slower in general, for all 4 scenarios tested.

I don't see this in PEHeader sources - it seek a offset only.

I didn't look into the PEHeader source, maybe that's caused by creating a FileStream.

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 File.ResolveLinkTarget and obviously one p/invoke SHGetFileInfo will be faster than PEHeader.

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 NativeCommandProcessor, and I don't know if that will change any of their decision. But if they documented the API, .NET team will update ResolveLinkTarget to support AppExecLink, so we can just add back File.ResolveLinkTarget when that day comes.

Can you maybe update your PR to remove File.ResolveLinkTarget in this code path?

Can you maybe update your PR to remove File.ResolveLinkTarget in this code path?

Too long path :-) Will do new PR.

Yeah, a new PR is better, making the purpose clearer too.
Please make sure the new PR description captures the gist of #16295 (comment). Thanks!

I will close this PR then.