Property check for IsGlobalProperty and IsEnvironmentProperty creates a copy of GlobalPropertiesDictionary for every call

[arkalyanms]

Issue Description

There are a few scenarios on the project system side that iterates over a property collection to check IsEnvironmentProperty and IsGlobalProperty. Inside msbuild, that checks _project.GlobalProperties.ContainsKey(name). project.GlobalProperties creates a new dictionary from GlobalPropertiesDictionary on every call. This gets expensive in operations that iterate over a collection of properties.

This impacts the first time solution close scenario for netcore projects

Would it make sense to cache the global property dictionary as a read only variant for the getter?

[rainersigwald]

At first glance that seems like it would be fine to me. . .

[Forgind]

I felt like looking into this, and I don't think it's doable perfectly without a breaking change. I don't know if it's a big breaking change; I'd guess small, but I'm not sure. Here's why:

My first thought was skipping making a second dictionary at all and adding a GlobalPropertiesContains function/property to ProjectImpl, but that would require changing ProjectLink as well, since Project technically only has a ProjectLink. ProjectLink can be passed in by the user, so any change in how it's accessed is a breaking change.

Doing as you actually suggested and caching a global property dictionary that you return when the user asks for them leave you two options: a ReadOnlyDictionary or a normal Dictionary that's readonly. A ReadOnlyDictionary satisfies our requirements here, and we never try to modify it, but since it's a different return value (ReadOnlyDictionary does not implement IDictionary), it'd still be a breaking change. Returning a readonly Dictionary permits the user to add or remove values at will. Note that GlobalProperties is a public property on Project, so the user can modify it. This means that, at present, the user can modify it at will, knowing it won't affect anything else. With this change, they couldn't.

One option is to create a GlobalPropertiesContains method on ProjectImpl and call that if our ProjectLink happens to be a ProjectImpl. From how I understand your use case, that would fix your problem. I'm happy doing that.

How does that sound @arkalyanms?

[lifengl]

I wonder you need a breaking change to introduce new method to ProjectLink. It is an abstract class, which allows us to define new virtual method (not abstract method), and default implementation can fall back to the original abstract property.

[Forgind]

We should be able to do that. Does that mean you are making your own ProjectLink rather than using our ProjImpl?

[lifengl]

ProjectLink is an abstract class, which allows other features to implement it differently, so it can simulate msbuild evaluation results like the result of a msbuild evaluation. The msbuild implements one through ProjImpl, which is used in build and also the project system. However, we at least have two more implementation of it, one is to allow project evaluation to happen in a different process, and ProjectLink allows consumer code to work with the evaluation result (while it's backend is in a different process, all calls into ProjectLink is forwarded to another process.) Another implementation is Arun's evaluation cache, the result of previous evaluation is read from file, and ProjectLink allows other code to access it like it is from a fresh evaluation.

Adding more abstract methods to this class would be a breaking change to more implementations. However, it is possible to add virtual methods which has default implementation to depend on other existing properties/methods, so existing implementation would work. However, your ProjImpl can override those virtual methods to provide more efficient implementation, as well as other implementation when they release a new version of the implementation with the updated base class.