A better GetAllGlobs #1871

cdmihai · 2017-03-16T05:24:09Z

Resolves #1795

This is based on another PR: #1859

rainersigwald

Would like to see much more commentary in the commit message. Why wasn't the previous implementation good enough and what does this provide?

rainersigwald · 2017-03-17T21:50:54Z

src/Build/Definition/Project.cs

@@ -1117,32 +1129,161 @@ public List<GlobResult> GetAllGlobs(string itemType)
            return GetAllGlobs(GetItemElementsByType(GetEvaluatedItemElements(), itemType));
        }

+        private struct CumulatedRemoveElementData


Accumulated? Or Cumulative?

Cumulative. As the items are scanned, that struct grows with every new piece of information on Remove operations. I'll rename it.

rainersigwald · 2017-03-17T22:20:01Z

src/Build/Definition/Project.cs

@@ -1117,32 +1129,161 @@ public List<GlobResult> GetAllGlobs(string itemType)
            return GetAllGlobs(GetItemElementsByType(GetEvaluatedItemElements(), itemType));
        }

+        private struct CumulatedRemoveElementData
+        {
+            public ImmutableList<IMSBuildGlob>.Builder Globs { get; set; }


Exposing a Builder as a public field feels wrong.

True, I could encapsulate it and provide CumulateRemoveGlob / GetImmutableSnapshot type of methods. However, since it's a private implementation struct I did not consider it too much (If I could have defined structs inside methods I would have done that). I'll play around with it and see hot it looks encapsulated.

rainersigwald · 2017-03-20T14:06:40Z

src/Build/Definition/Project.cs

+            // scan the project elements in reverse order and build globbing information for each include element
+            // based on the fact that relevant removes for a particular include element (element A) consist of:
+            // - all the removes seen by the next include statement of A's type (element B which appears after A in file order)
+            // - new removes between A and B


Between A and B, or just the one for A?

Removes that apply to A but not to B. Spacially, these are placed between A's element and B's element.

1. <A Include="A"/> 2. <A Remove="..."/> // this remove applies to the A includes 3. <A Include="B"/> 4. <A Remove="..."/> // this remove applies to the A and B includes 5. <A Include="C"/> 6. <A Remove="..."/> // this remove applies to the A, B, and C includes

So A's applicable removes are composed of:

all the removes seen by the next include statement of A's type (element B which appears after A in file order). In this example that's Removes 4 and 6.

new removes between A and B. In this example that's Remove 2.

rainersigwald · 2017-03-20T14:08:17Z

src/Build/Definition/Project.cs

-                            f => !(f is ItemExpressionFragment<ProjectProperty, ProjectItem>))
-                            .Select(f => f.ItemSpecFragment)
-                            .ToImmutableHashSet());
+            var includeGlobFragments = includeItemspec.Fragments.Where(f => f is GlobFragment).ToImmutableArray();


Does this need to be immutable? looks like you could get away with just an array, which might be faster?

I used this as documentation: https://blogs.msdn.microsoft.com/dotnet/2013/06/24/please-welcome-immutablearrayt/
ImmutableArray is backed up by an actual array, and it's a struct. Since there does not seem to be a obvious perf difference from the regular array, I went with the more expressive choice. Is there something I'm missing? :)

Ah, I didn't know that was quite so nice. Leave it, then--I like communicating the intent through the type.

rainersigwald · 2017-03-20T14:15:26Z

src/Build/Definition/Project.cs

+            }
+        }
+
+        private IEnumerable<string> GetRelevantFragmentStringsForExcludesAndRemoves(EvaluationItemSpec spec)


Needs documentation:

Reorders the fragments

What fragments does it not return?

"Relevant" to what?

rainersigwald · 2017-03-20T14:17:08Z

src/Build/Evaluation/ItemSpec.cs

+        /// <returns></returns>
+        public IMSBuildGlob ToMSBuildGlob()
+        {
+            return new CompositeGlob(Fragments.Select(f => f.ToMSBuildGlob()).ToImmutableArray());


This calling pattern makes me think a ctor should take an IEnumerable and immutable-ize it itself.

Resolves dotnet#1795 GlobResult needs to offer a globbing object that CPS can match arbitrary strings against. The glob needed to include all the globs from a project item element's include attribute, and exclude all the itemspec fragments from the corresponding Exclude and all subsequent Remove elements that apply to that include. This required changing GetAllGlobs such that it incorporates information from Removes and constructs MSBuildGlobWithGaps objects for each include operation with globs.

CompositeGlob needs to be immutable, so entering objects need to be cloned / immutable. Immutable arrays because they are fast to iterate and don't add a wrapping object.

cdmihai · 2017-03-21T00:26:17Z

@rainersigwald Implemented your feedback.

rainersigwald

More nits because I'm me, but looking good I think. Biggest question is about breaking changes to the public interface.

rainersigwald · 2017-03-21T14:43:10Z

ref/net46/Microsoft.Build/Microsoft.Build.cs

        public System.Collections.Generic.IEnumerable<string> Excludes { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
-        public string Glob { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }


These are breaking changes. What's the strategy for dealing with that?

I could unbreak it by:

keeping the constructor public and overloading it

mark Glob as deprecated and have it return the first glob. After this change, there is one GlobResult per item element with globs. Before, there was one GlobResult per glob fragment, so one item element could have produced multiple GlobResults. So users of this member would break at run time rather than compile time, as they do now.

As an alternative, I could keep the old code and have the new variants as GetAllGlobs2 and GlobResult2 :)

I searched on Github and Index and found no other usage other than CPS. So breaking it looks like a viable option, since the other options introduce baggage.

rainersigwald · 2017-03-21T14:46:20Z

src/Build/Globbing/CompositeGlob.cs

-        /// <param name="globs">children globs</param>
-        public CompositeGlob(params IMSBuildGlob[] globs) : this(globs.AsEnumerable())
+        /// <param name="globs">Children globs. Input gets shallow cloned</param>
+        public CompositeGlob(params IMSBuildGlob[] globs) : this(globs.ToImmutableArray())


What's the impact of doing ToImmutableArray() twice when calling through this pattern to the other?

None: https://source.dot.net/#System.Collections.Immutable/System/Collections/Immutable/ImmutableArray.cs,390. I thought about duplicating the check in case ImmutableArray changes and removes its own check, but I decided against it. "It will never happen" :)

Sweet, works for me!

rainersigwald · 2017-03-21T15:29:29Z

src/Build/Definition/Project.cs

+            public IEnumerable<IMSBuildGlob> Globs => _globs.ToImmutable();
+            public IEnumerable<string> FragmentStrings => _fragmentStrings.ToImmutable();
+
+            public static CumulativeRemoveElementData New()


In .NET this pattern is usually named .Create().

rainersigwald · 2017-03-21T15:29:49Z

src/Build/Definition/Project.cs

+            private ImmutableList<IMSBuildGlob>.Builder _globs;
+            private ImmutableHashSet<string>.Builder _fragmentStrings;
+
+            public IEnumerable<IMSBuildGlob> Globs => _globs.ToImmutable();


If you're returning an IEnumerable is there any point in making it immutable? It's already readonly.

Sanity. Just in case someone casts it back to builder and mutates it :)
The operation itself is not that expensive, it traverses the backing tree and toggles a _frozen bool to true. That and creating a new wrapper object.

later edit: oh, and the builder keeps on mutating after this code is called. The structural sharing then makes sure there's no memory bloating.

rainersigwald · 2017-03-21T15:30:11Z

src/Build/Definition/Project.cs

+                };
+            }
+
+            public void CumulateInformationFromRemoveItemSpec(EvaluationItemSpec removeSpec)


Nit: Accumulate

rainersigwald · 2017-03-21T15:35:10Z

src/Build/Definition/Project.cs

+            // So A's applicable removes are composed of:
+            // 
+            // The applicable removes for the element at position 1. are composed of:
+            // - all the removes seen by the next include statement of A's type (element B which appears after A in file order). In this example that's Removes 4 and 6.


Ahhhhh I think I understand now. Can you rename the item itself from A to I in the example and comments? I was getting confused between A-the-item-type and A-the-included-item.

I'm also a bit confused by what you mean by element B here. Line 3 that included B?

xml element B. I'll update the comment

rainersigwald · 2017-03-21T15:40:46Z

src/Build/Definition/Project.cs

+            // - new removes between A and B. In this example that's Remove 2.
+
+            // use immutable lists because there will be a lot of structural sharing between includes which share increasing subsets of corresponding remove elements
+            // item type -> aggregated information about all removes seen so far for that item type


Should this go elsewhere now? there's no immutable right here.

I'd keep it as a comment on the reason why immutable types are really good here.

cdmihai · 2017-03-21T18:35:17Z

@mfilippov / @mhutch FYI, this PR will introduce a breaking change in GetAllGlobs / GlobResult, in case you are using it.

mfilippov · 2017-03-21T18:39:34Z

@cdmihai Thanks for information.

mhutch · 2017-03-21T21:33:39Z

@cdmihai thanks for the heads up!

cc @slluis.

This reverts commit 1b2c9de.

This reverts commit b98bc3a.

* Revert "Revert "A better GetAllGlobs (#1871)" (#1946)" This reverts commit b98bc3a.

dnfclas added the cla-already-signed label Mar 16, 2017

cdmihai force-pushed the aBetterGetAllGlobs branch from 8759774 to 6a00580 Compare March 16, 2017 23:57

cdmihai added 3 commits March 17, 2017 14:26

Alias generic type

f89f7a3

ItemExpressionFragment provides referenced items

956381f

ItemSpec knows how to convert itself to IMSBuildGlob

2fe536e

cdmihai force-pushed the aBetterGetAllGlobs branch from 6a00580 to 646034a Compare March 17, 2017 21:27

rainersigwald requested changes Mar 20, 2017

View reviewed changes

cdmihai added 4 commits March 20, 2017 14:18

propagate globAPI PR feedback

d1467af

update APIs

f89aef3

consistency renames

a72a504

cdmihai force-pushed the aBetterGetAllGlobs branch from c149cb1 to a72a504 Compare March 20, 2017 21:24

cdmihai added 5 commits March 20, 2017 14:55

comments

9812091

Rename and document method

b75af41

CompositeGlob ctor clones input in ImmutableArray

3ba242f

CompositeGlob needs to be immutable, so entering objects need to be cloned / immutable. Immutable arrays because they are fast to iterate and don't add a wrapping object.

Move method to ItemSpec

50874da

Encapsulate CumulativeRemoveElementData

024979e

rainersigwald approved these changes Mar 21, 2017

View reviewed changes

cdmihai added 3 commits March 21, 2017 10:15

sanity check

118ae61

update comments

8f5218b

renames

49ee111

AndyGerlicher approved these changes Mar 22, 2017

View reviewed changes

cdmihai merged commit 1b2c9de into dotnet:master Mar 22, 2017

cdmihai added a commit to cdmihai/msbuild that referenced this pull request Apr 5, 2017

Revert "A better GetAllGlobs (dotnet#1871)"

b06a3d0

This reverts commit 1b2c9de.

AndyGerlicher pushed a commit that referenced this pull request Apr 5, 2017

Revert "A better GetAllGlobs (#1871)" (#1946)

b98bc3a

This reverts commit 1b2c9de.

cdmihai added a commit that referenced this pull request Apr 6, 2017

Revert "Revert "A better GetAllGlobs (#1871)" (#1946)"

3fe0039

This reverts commit b98bc3a.

cdmihai added a commit to cdmihai/msbuild that referenced this pull request Apr 18, 2017

Revert "Revert "A better GetAllGlobs (dotnet#1871)" (dotnet#1946)"

07aa468

This reverts commit b98bc3a.

cdmihai added a commit that referenced this pull request Apr 20, 2017

Revert "Revert "A better GetAllGlobs (#1871)" (#1946)" (#1989)

8c9db36

* Revert "Revert "A better GetAllGlobs (#1871)" (#1946)" This reverts commit b98bc3a.

cdmihai deleted the aBetterGetAllGlobs branch May 12, 2017 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A better GetAllGlobs #1871

A better GetAllGlobs #1871

cdmihai commented Mar 16, 2017 •

edited

Loading

rainersigwald left a comment

rainersigwald Mar 17, 2017

cdmihai Mar 20, 2017

rainersigwald Mar 17, 2017

cdmihai Mar 20, 2017

rainersigwald Mar 20, 2017

cdmihai Mar 20, 2017

rainersigwald Mar 20, 2017

cdmihai Mar 20, 2017

rainersigwald Mar 20, 2017

rainersigwald Mar 20, 2017

rainersigwald Mar 20, 2017

cdmihai commented Mar 21, 2017

rainersigwald left a comment

rainersigwald Mar 21, 2017

cdmihai Mar 21, 2017 •

edited

Loading

rainersigwald Mar 21, 2017

cdmihai Mar 21, 2017 •

edited

Loading

rainersigwald Mar 21, 2017

rainersigwald Mar 21, 2017

rainersigwald Mar 21, 2017

cdmihai Mar 21, 2017 •

edited

Loading

rainersigwald Mar 21, 2017

rainersigwald Mar 21, 2017

cdmihai Mar 21, 2017

rainersigwald Mar 21, 2017

cdmihai Mar 21, 2017

cdmihai commented Mar 21, 2017

mfilippov commented Mar 21, 2017

mhutch commented Mar 21, 2017

		public System.Collections.Generic.IEnumerable<string> Excludes { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
		public string Glob { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }

A better GetAllGlobs #1871

A better GetAllGlobs #1871

Conversation

cdmihai commented Mar 16, 2017 • edited Loading

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai commented Mar 21, 2017

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai commented Mar 21, 2017

mfilippov commented Mar 21, 2017

mhutch commented Mar 21, 2017

cdmihai commented Mar 16, 2017 •

edited

Loading

cdmihai Mar 21, 2017 •

edited

Loading

cdmihai Mar 21, 2017 •

edited

Loading

cdmihai Mar 21, 2017 •

edited

Loading