Skip to content
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

MSBuild throws thousands of first-chance exceptions evaluating projects with certain types of property functions #2217

Open
davkean opened this issue Jun 14, 2017 · 18 comments
Labels
performance Performance-Scenario-General This issue affects performance in general. Priority:2 Work that is important, but not critical for the release triaged

Comments

@davkean
Copy link
Member

davkean commented Jun 14, 2017

MSBuild is throwing thousands of exceptions opening certain solutions which this makes debugging VS extremely painful and might affect the loading time of these solutions.

Opening Roslyn.sln, MSBuild threw over > 21,000 first chance exceptions with stacks similar to:

MissingMethodException: Attempted to access a missing member.
mscorlib.dll!System.DefaultBinder.BindToMethod(System.Reflection.BindingFlags bindingAttr, System.Reflection.MethodBase[] match, ref object[] args, System.Reflection.ParameterModifier[] modifiers, System.Globalization.CultureInfo cultureInfo, string[] names, out object state)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.Function<Microsoft.Build.Evaluation.ProjectProperty>.Execute(object objectInstance, Microsoft.Build.Evaluation.IPropertyProvider<Microsoft.Build.Evaluation.ProjectProperty> properties, Microsoft.Build.Evaluation.ExpanderOptions options, Microsoft.Build.Shared.IElementLocation elementLocation)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.PropertyExpander<Microsoft.Build.Evaluation.ProjectProperty>.ExpandPropertyBody(string propertyBody, object propertyValue, Microsoft.Build.Evaluation.IPropertyProvider<Microsoft.Build.Evaluation.ProjectProperty> properties, Microsoft.Build.Evaluation.ExpanderOptions options, Microsoft.Build.Shared.IElementLocation elementLocation, Microsoft.Build.Evaluation.UsedUninitializedProperties usedUninitializedProperties)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.Function<Microsoft.Build.Evaluation.ProjectProperty>.Execute(object objectInstance, Microsoft.Build.Evaluation.IPropertyProvider<Microsoft.Build.Evaluation.ProjectProperty> properties, Microsoft.Build.Evaluation.ExpanderOptions options, Microsoft.Build.Shared.IElementLocation elementLocation)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.PropertyExpander<Microsoft.Build.Evaluation.ProjectProperty>.ExpandPropertyBody(string propertyBody, object propertyValue, Microsoft.Build.Evaluation.IPropertyProvider<Microsoft.Build.Evaluation.ProjectProperty> properties, Microsoft.Build.Evaluation.ExpanderOptions options, Microsoft.Build.Shared.IElementLocation elementLocation, Microsoft.Build.Evaluation.UsedUninitializedProperties usedUninitializedProperties)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.PropertyExpander<Microsoft.Build.Evaluation.ProjectProperty>.ExpandPropertiesLeaveTypedAndEscaped(string expression, Microsoft.Build.Evaluation.IPropertyProvider<Microsoft.Build.Evaluation.ProjectProperty> properties, Microsoft.Build.Evaluation.ExpanderOptions options, Microsoft.Build.Shared.IElementLocation elementLocation, Microsoft.Build.Evaluation.UsedUninitializedProperties usedUninitializedProperties)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.Function<Microsoft.Build.Evaluation.ProjectProperty>.Execute(object objectInstance, Microsoft.Build.Evaluation.IPropertyProvider<Microsoft.Build.Evaluation.ProjectProperty> properties, Microsoft.Build.Evaluation.ExpanderOptions options, Microsoft.Build.Shared.IElementLocation elementLocation)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.PropertyExpander<Microsoft.Build.Evaluation.ProjectProperty>.ExpandPropertyBody(string propertyBody, object propertyValue, Microsoft.Build.Evaluation.IPropertyProvider<Microsoft.Build.Evaluation.ProjectProperty> properties, Microsoft.Build.Evaluation.ExpanderOptions options, Microsoft.Build.Shared.IElementLocation elementLocation, Microsoft.Build.Evaluation.UsedUninitializedProperties usedUninitializedProperties)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.PropertyExpander<Microsoft.Build.Evaluation.ProjectProperty>.ExpandPropertiesLeaveTypedAndEscaped(string expression, Microsoft.Build.Evaluation.IPropertyProvider<Microsoft.Build.Evaluation.ProjectProperty> properties, Microsoft.Build.Evaluation.ExpanderOptions options, Microsoft.Build.Shared.IElementLocation elementLocation, Microsoft.Build.Evaluation.UsedUninitializedProperties usedUninitializedProperties)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.PropertyExpander<Microsoft.Build.Evaluation.ProjectProperty>.ExpandPropertiesLeaveEscaped(string expression, Microsoft.Build.Evaluation.IPropertyProvider<Microsoft.Build.Evaluation.ProjectProperty> properties, Microsoft.Build.Evaluation.ExpanderOptions options, Microsoft.Build.Shared.IElementLocation elementLocation, Microsoft.Build.Evaluation.UsedUninitializedProperties usedUninitializedProperties)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem>.ExpandIntoStringLeaveEscaped(string expression, Microsoft.Build.Evaluation.ExpanderOptions options, Microsoft.Build.Shared.IElementLocation elementLocation)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.EvaluatePropertyElement(Microsoft.Build.Construction.ProjectPropertyElement propertyElement)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.EvaluatePropertyGroupElement(Microsoft.Build.Construction.ProjectPropertyGroupElement propertyGroupElement)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.PerformDepthFirstPass(Microsoft.Build.Construction.ProjectRootElement currentProjectOrImport)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.EvaluateImportElement(string directoryOfImportingFile, Microsoft.Build.Construction.ProjectImportElement importElement)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.PerformDepthFirstPass(Microsoft.Build.Construction.ProjectRootElement currentProjectOrImport)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.EvaluateImportElement(string directoryOfImportingFile, Microsoft.Build.Construction.ProjectImportElement importElement)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.PerformDepthFirstPass(Microsoft.Build.Construction.ProjectRootElement currentProjectOrImport)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.EvaluateImportElement(string directoryOfImportingFile, Microsoft.Build.Construction.ProjectImportElement importElement)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.PerformDepthFirstPass(Microsoft.Build.Construction.ProjectRootElement currentProjectOrImport)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.Evaluate()	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition>.Evaluate(Microsoft.Build.Evaluation.IEvaluatorData<Microsoft.Build.Evaluation.ProjectProperty, Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectMetadata, Microsoft.Build.Evaluation.ProjectItemDefinition> data, Microsoft.Build.Construction.ProjectRootElement root, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings, int maxNodeCount, Microsoft.Build.Collections.PropertyDictionary<Microsoft.Build.Execution.ProjectPropertyInstance> environmentProperties, Microsoft.Build.BackEnd.Logging.ILoggingService loggingService, Microsoft.Build.Evaluation.IItemFactory<Microsoft.Build.Evaluation.ProjectItem, Microsoft.Build.Evaluation.ProjectItem> itemFactory, Microsoft.Build.Evaluation.IToolsetProvider toolsetProvider, Microsoft.Build.Evaluation.ProjectRootElementCache projectRootElementCache, Microsoft.Build.Framework.BuildEventContext buildEventContext, Microsoft.Build.Execution.ProjectInstance projectInstanceIfAnyForDebuggerOnly, Microsoft.Build.BackEnd.SdkResolution sdkResolution)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Project.Reevaluate(Microsoft.Build.BackEnd.Logging.ILoggingService loggingServiceForEvaluation, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Project.ReevaluateIfNecessary(Microsoft.Build.BackEnd.Logging.ILoggingService loggingServiceForEvaluation, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Project.Initialize(System.Collections.Generic.IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Project.Project(Microsoft.Build.Construction.ProjectRootElement xml, System.Collections.Generic.IDictionary<string, string> globalProperties, string toolsVersion, string subToolsetVersion, Microsoft.Build.Evaluation.ProjectCollection projectCollection, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings)	Unknown
Microsoft.Build.dll!Microsoft.Build.Evaluation.Project.Project(Microsoft.Build.Construction.ProjectRootElement xml, System.Collections.Generic.IDictionary<string, string> globalProperties, string toolsVersion, Microsoft.Build.Evaluation.ProjectCollection projectCollection, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings)	Unknown
InvalidCastException: Invalid cast from 'System.String' to 'System.Int32[]'
Microsoft.Build.dll!Function`1.CoerceArguments(object[] args = {unknown}, System.Reflection.ParameterInfo[] parameters = {unknown})	C#
Microsoft.Build.dll!Function`1.LateBindExecute(System.Exception ex = {unknown}, System.Reflection.BindingFlags bindingFlags = {unknown}, object objectInstance = {unknown}, object[] args = {unknown}, bool isConstructor = {unknown})	C#
Microsoft.Build.dll!Function`1.Execute(object objectInstance = {unknown}, Microsoft.Build.Evaluation.IPropertyProvider properties = {unknown}, Microsoft.Build.Evaluation.ExpanderOptions options = {unknown}, Microsoft.Build.Shared.IElementLocation elementLocation = {unknown})	C#
Microsoft.Build.dll!PropertyExpander`1.ExpandPropertyBody(string propertyBody = {unknown}, object propertyValue = {unknown}, Microsoft.Build.Evaluation.IPropertyProvider properties = {unknown}, Microsoft.Build.Evaluation.ExpanderOptions options = {unknown}, Microsoft.Build.Shared.IElementLocation elementLocation = {unknown}, Microsoft.Build.Evaluation.UsedUninitializedProperties usedUninitializedProperties = {unknown})	C#
Microsoft.Build.dll!Function`1.Execute(object objectInstance = {unknown}, Microsoft.Build.Evaluation.IPropertyProvider properties = {unknown}, Microsoft.Build.Evaluation.ExpanderOptions options = {unknown}, Microsoft.Build.Shared.IElementLocation elementLocation = {unknown})	C#
Microsoft.Build.dll!PropertyExpander`1.ExpandPropertyBody(string propertyBody = {unknown}, object propertyValue = {unknown}, Microsoft.Build.Evaluation.IPropertyProvider properties = {unknown}, Microsoft.Build.Evaluation.ExpanderOptions options = {unknown}, Microsoft.Build.Shared.IElementLocation elementLocation = {unknown}, Microsoft.Build.Evaluation.UsedUninitializedProperties usedUninitializedProperties = {unknown})	C#
Microsoft.Build.dll!PropertyExpander`1.ExpandPropertiesLeaveTypedAndEscaped(string expression = {unknown}, Microsoft.Build.Evaluation.IPropertyProvider properties = {unknown}, Microsoft.Build.Evaluation.ExpanderOptions options = {unknown}, Microsoft.Build.Shared.IElementLocation elementLocation = {unknown}, Microsoft.Build.Evaluation.UsedUninitializedProperties usedUninitializedProperties = {unknown})	C#
Microsoft.Build.dll!PropertyExpander`1.ExpandPropertiesLeaveEscaped(string expression = {unknown}, Microsoft.Build.Evaluation.IPropertyProvider properties = {unknown}, Microsoft.Build.Evaluation.ExpanderOptions options = {unknown}, Microsoft.Build.Shared.IElementLocation elementLocation = {unknown}, Microsoft.Build.Evaluation.UsedUninitializedProperties usedUninitializedProperties = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Expander`2.ExpandIntoStringLeaveEscaped(string expression = {unknown}, Microsoft.Build.Evaluation.ExpanderOptions options = {unknown}, Microsoft.Build.Shared.IElementLocation elementLocation = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.EvaluatePropertyElement(Microsoft.Build.Construction.ProjectPropertyElement propertyElement = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.EvaluatePropertyGroupElement(Microsoft.Build.Construction.ProjectPropertyGroupElement propertyGroupElement = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.PerformDepthFirstPass(Microsoft.Build.Construction.ProjectRootElement currentProjectOrImport = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.EvaluateImportElement(string directoryOfImportingFile = {unknown}, Microsoft.Build.Construction.ProjectImportElement importElement = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.PerformDepthFirstPass(Microsoft.Build.Construction.ProjectRootElement currentProjectOrImport = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.EvaluateImportElement(string directoryOfImportingFile = {unknown}, Microsoft.Build.Construction.ProjectImportElement importElement = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.PerformDepthFirstPass(Microsoft.Build.Construction.ProjectRootElement currentProjectOrImport = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.EvaluateImportElement(string directoryOfImportingFile = {unknown}, Microsoft.Build.Construction.ProjectImportElement importElement = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.PerformDepthFirstPass(Microsoft.Build.Construction.ProjectRootElement currentProjectOrImport = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.Evaluate()	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Evaluator`4.Evaluate(Microsoft.Build.Evaluation.IEvaluatorData data = {unknown}, Microsoft.Build.Construction.ProjectRootElement root = {unknown}, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings = {unknown}, int maxNodeCount = {unknown}, Microsoft.Build.Collections.PropertyDictionary<Microsoft.Build.Execution.ProjectPropertyInstance> environmentProperties = {unknown}, Microsoft.Build.BackEnd.Logging.ILoggingService loggingService = {unknown}, Microsoft.Build.Evaluation.IItemFactory itemFactory = {unknown}, Microsoft.Build.Evaluation.IToolsetProvider toolsetProvider = {unknown}, Microsoft.Build.Evaluation.ProjectRootElementCache projectRootElementCache = {unknown}, Microsoft.Build.Framework.BuildEventContext buildEventContext = {unknown}, Microsoft.Build.Execution.ProjectInstance projectInstanceIfAnyForDebuggerOnly = {unknown}, Microsoft.Build.BackEnd.SdkResolution sdkResolution = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Project.Reevaluate(Microsoft.Build.BackEnd.Logging.ILoggingService loggingServiceForEvaluation = {unknown}, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Project.ReevaluateIfNecessary(Microsoft.Build.BackEnd.Logging.ILoggingService loggingServiceForEvaluation = {unknown}, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Project.Initialize(System.Collections.Generic.IDictionary<string,string> globalProperties = {unknown}, string toolsVersion = {unknown}, string subToolsetVersion = {unknown}, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Project..ctor(Microsoft.Build.Construction.ProjectRootElement xml = {unknown}, System.Collections.Generic.IDictionary<string,string> globalProperties = {unknown}, string toolsVersion = {unknown}, string subToolsetVersion = {unknown}, Microsoft.Build.Evaluation.ProjectCollection projectCollection = {unknown}, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings = {unknown})	C#
Microsoft.Build.dll!Microsoft.Build.Evaluation.Project..ctor(Microsoft.Build.Construction.ProjectRootElement xml = {unknown}, System.Collections.Generic.IDictionary<string,string> globalProperties = {unknown}, string toolsVersion = {unknown}, Microsoft.Build.Evaluation.ProjectCollection projectCollection = {unknown}, Microsoft.Build.Evaluation.ProjectLoadSettings loadSettings = {unknown})	C#

The following shows the timeline of loading the Roslyn solution:

image

Almost all of thrown exceptions above originate from and get handled in MSBuild.

The expressions that seem be causing these exceptions live in https:/dotnet/roslyn/blob/master/build/Targets/Versions.props:

    <BuildNumberFiveDigitDateStamp>$([MSBuild]::Subtract($(BuildNumber.Split('.')[0].Substring(3).Trim()), 8800))</BuildNumberFiveDigitDateStamp>
    <BuildNumberBuildOfTheDayPadded>$(BuildNumber.Split('.')[1].PadLeft(2,'0'))</BuildNumberBuildOfTheDayPadded>**

Can we please implement a way to avoid these exceptional paths?

@davkean
Copy link
Member Author

davkean commented Jun 14, 2017

I noticed while debugging that for [0] MSBuild is looking for a method called GetValue on the string[] instance that fails.

Update: Okay, I can see what's happening in this case, basically all binds to int indexers are guaranteed to fail on the first lookup because type argument type passed to InvokeMember in Expander<P, L>.Function.Execute is a "string", at least optimizing that case would help a lot.

@davkean
Copy link
Member Author

davkean commented Jun 14, 2017

This also happens 4 or 5 times by default in .NET Core apps, for example, expression is throwing: https:/dotnet/sdk/blob/a4bceb67b160c72318a581d87d52796e4fa31794/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.TargetFrameworkInference.targets#L51, which causes it to go down LateBindExecute to coerce the type to the right type.

Wonder if it's better to pass an implementation of `System.Reflection.Binder' that itself does the coercing of the type (via the abstract Binder.ChangeType) and implements MSBuild rules instead of doing the coercing after it throws.

@davkean
Copy link
Member Author

davkean commented Jun 14, 2017

Did some more research, basically implementing your own Binder is almost exactly what you want to implement here, it lets you:

  1. Given the same method name and the same number of arguments, pick from a list of available overloads to invoke (override BindToMethod)
  2. Manipulate the order of the arguments that are passed to the method (override BindToMethod returning a state, and override ReorderArgumentArray)
  3. Coerce arguments to different types based on the destination parameter type, (override ChangeType) - this will let you say change String.Substring("1") -> String.Substring(1) avoiding that first MissingMethodException.

My first thought was to implement a custom binder, first delegating to the default binder's implementation, then implementing the fallback logic if that fails. However, BindToMethod itself throws an exception if can't find the method so that rules it out.

That leaves a few options:

  1. Ask CoreFx to provide a non-throwing version of this API to avoid having to reimplement DefaultBinder.BindToMethod's logic (which looks complicated!).
  2. Reimplement DefaultBinder.BindToMethod's logic in a non-throwing way and then fall back to your usual fallback logic.
  3. Call DefaultBinder.SelectMethod, which implements half of BindToMethod's implementation of selecting a method based on a types, then fall back to your usuall fallback logic if that returns null

It appears that most of BindToMethod's logic handles significantly more than you need; VarArgs, Named Parameters. Given property functions are limited in the scope (by not being open ended), it looks like Type.DefaultBinder.SelectMethod will will do almost everything you need to maintain behavior (params might needed handled specially?), here's an example of what I'm thinking:

    public class MSBuildBinder : Binder
    {
        public override MethodBase BindToMethod(BindingFlags bindingAttr, MethodBase[] match, ref object[] args, ParameterModifier[] modifiers, CultureInfo culture, string[] names, out object state)
        {
            state = null;

            // Maintain compat
            MethodBase method = Type.DefaultBinder.SelectMethod(bindingAttr, match, args.Select(a => a.GetType()).ToArray(), modifiers);
            if (method != null)
                return method;

            // Implement Function<T>.LateBindExecute here

            return null;
        }

        public override object ChangeType(object value, Type type, CultureInfo culture)
        {
            // Implement rest of Function<T>.CoerceArguments here

            return Convert.ChangeType(value, type, culture);
        }
   [...]
}

One more thing, it appears that you support a case-insensitive bind, you can do that simply by passing BindingFlags.IgnoreCase to InvokeMethod.

@lifengl
Copy link

lifengl commented Aug 23, 2017

+1

It causes many problems to debug, and when a debugger is attached, it makes the project system so slow that it is sometime not usable.

@davkean
Copy link
Member Author

davkean commented Sep 19, 2017

I'm marking this as having a performance impact, in investigating the cost of dotnet/project-system#2823, I setup a project that had 1000 of the same expressions, and found it took 7 seconds to evaluate of which 60% of CPU of building the project was just in binding the methods including ~43% just throwing exceptions

image
image

1000 might seem a lot of expressions above, but as you can see in the Roslyn case there's >21,000 attempts to bind, so there's clearly going to be huge amount of time spend on this.

@nguerrera
Copy link
Contributor

Note that on .NET Core, the custom binder is going to require moving to .netstandard/.netcoreapp 2.0. Can we please prioritize that and not put this behind another netfx ifdef? Core msbuild is currently significantly slower than desktop msbuild and several performance issues have been from suboptimal code under ifdef to work around API that were not available in 1.x.

@davkean
Copy link
Member Author

davkean commented Sep 19, 2017

Given there's a non-trivial overhead of Binder itself, a custom binder might not be the way to go - I'll investigate tomorrow.

@davkean
Copy link
Member Author

davkean commented Oct 2, 2017

@cdmihai said the following in #134:

We should redo the logic to avoid the exception. Maybe directly reflect into the receiver's type and match the method instead of first blindly calling it with strings.

The caveat here is the nature of data. If most of the time builds end up calling intrinsic functions which only take strings, then the current "exceptional" code would actually be faster than reflecting into the receiver to find the proper argument types. We should first build an empty console project and some real, big projects to get a sense of the landscape.

@rainersigwald said the following:

It might also be interesting to build a cache, so the first time we see Substring we do the slow path but the next time we see it, we try the thing that worked last time first and fall back to the slow path.
We could also potentially build our knowledge of these functions--right now we don't know much about the functions we can call, which is nice from a layering perspective but we could probably have higher performance if we had more knowledge of common calls.

@KirillOsenkov
Copy link
Member

I took a stab at a tactical fix that gets most common exceptions out of the way:
#2833

From my initial testing this completely removes first-chance exceptions when evaluating Roslyn and several other large codebases. It's super simple to add more methods here as we encounter them. We can take this fix as a stopgap while we wait for a proper solution. It removes the pain and shows ~30x perf improvement compared to binding with exceptions (~1.5ms vs. ~0.05 ms).

@KirillOsenkov
Copy link
Member

A large chunk of these should be gone, but I wouldn't call my change "systemic fix". More of a tactical fix to stop the bleeding.

We still need to do something about the binder long term.

@aolszowka
Copy link

We're getting burned by this; it appears that the Well Known methods did not include the following (which are burning us now):

Remove(Int32)
IndexOf() [For us specifically (char,Int32) and (char)]

I believe we'd need more cases here:
https:/microsoft/msbuild/blob/eaac97c461567fd7f07d1583898f16ebdad412b7/src/Build/Evaluation/Expander.cs#L3474

Here's a snippet of places burning us:

    <VAR2>$([MSBuild]::Add($(UnevaluatedOutputEnvironmentVariablePosition_), 0))</VAR2>
    <UnevaluatedOutputEnvironmentVariable_ Condition="'$(UnevaluatedOutputEnvironmentVariablePosition_)' != '' and $(UnevaluatedOutputEnvironmentVariablePosition_) > 1">$(UnevaluatedOutputPath.Remove($(VAR2)).ToUpper())</UnevaluatedOutputEnvironmentVariable_>

As well as:

    <VAR4>$([MSBuild]::Add($(CombinedEnvVarEvalEqualsPosition_), 1))</VAR4>
    <CombinedEnvVarEvalSemicolonPosition_ Condition="'$(UnevaluatedOutputEnvironmentVariable_)' != ''">$(UpperCombinedEnvVars_.IndexOf(';', $(VAR4)))</CombinedEnvVarEvalSemicolonPosition_>

MSbuild.exe reports as:

C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise>msbuild
Microsoft (R) Build Engine version 15.9.21+g9802d43bc3 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

This is actually a third party VS extension to us; is there any advice you can give to the vendor to work around this short of waiting for a fix to MSBuild?

@rainersigwald
Copy link
Member

@aolszowka Can you describe the impact you're seeing? This causes some slower evaluation performance, but I wouldn't expect it to be a hot problem for anyone in the grand scheme of things.

@madkat
Copy link
Contributor

madkat commented Jul 31, 2019

@rainersigwald Third party VS extension checking in. We abuse that pattern frequently to translate some of our proprietary project system settings into more palatable values for MSBuild consumption. The impact on @aolszowka is compounded by both his volume of projects as well as hard ProjectCollection.LoadProject operations that occur inside our build tasks (which are not making use of the caching mechanisms of MSBuild).

I have a clear path forward on improving build performance for my customer, but I am curious if there is something I could be doing differently to avoid the exceptions altogether in MSBuild 15. We see the massive exception volume every time we debug project loads inside of our VS extension.

@aolszowka
Copy link

We're seeing over 30,000 FCE's just on Initial Solution Execution in MSBuild; to compound issues the vendor is currently performing a hard load of every dependent project (via Project.LoadProject(string)). The vendor is working to remove the hard project loads which will help the additional FCE's but the initial ramp up of attempting to debug is extremely painful.

image

Its just a constant one after another here:

image

@rainersigwald
Copy link
Member

I see, thanks.

The only way to avoid the impact is to avoid using property functions that aren't in the optimized list, which is this for 15.9:

https:/microsoft/msbuild/blob/v15.9.21.664/src/Build/Evaluation/Expander.cs#L3412-L3811

For Remove(x), you could probably use Substring(0, x):

https:/microsoft/msbuild/blob/9802d43bc30b997b75c5efa310a9d379bd56162f/src/Build/Evaluation/Expander.cs#L3474-L3494

I don't see a similar hack for IndexOf(int, int).

Can you defer these calculations out of evaluation time to build time by putting them in a target? The only other thing I can think of is precomputing them in another imported file, but that could get messy quick.

@aolszowka
Copy link

@rainersigwald is there a plan for a fix for VS2019? We're evaluating upgrading at this time and were hoping to hit it soon-ish (3-6 months). Several fixes up there are already attractive to us (most notably the longpath support which we're already hacking around) so it may be a non-issue if we can get there.

@madkat
Copy link
Contributor

madkat commented Jul 31, 2019

@rainersigwald Moving the calculations into a target is probably immediately doable for most of them. Sounds like a good opportunity for some refactoring if that is a viable answer. Thanks for the suggestion.

@rainersigwald
Copy link
Member

We'd happily take a PR to add these to the list of known fast-path property functions (for 16.4, probably). You're also welcome to file a new bug asking us to do that and we'll prioritize it against other work. I do not expect to overhaul property function expansion to fix the problem entirely in the short- or medium term.

@panopticoncentral panopticoncentral added the Priority:2 Work that is important, but not critical for the release label Mar 23, 2021
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance-Scenario-General This issue affects performance in general. Priority:2 Work that is important, but not critical for the release triaged
Projects
None yet
Development

No branches or pull requests