Optimize indexer access of an inline range expression by agocke · Pull Request #36147 · dotnet/roslyn

agocke · 2019-06-04T06:25:26Z

Introduces optimizations in lowering for an indexer access of the form
receiver[a..b]. With the optimizations, the generation of intermediate
System.Index and System.Range types will be avoided.

Introduces optimizations in lowering for an indexer access of the form receiver[a..b]. With the optimizations, the generation of intermediate System.Index and System.Range types will be avoided.

RikkiGibson · 2019-06-04T23:53:14Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs

+                }
+
+                BoundExpression endExpr;
+                switch (rangeExpr.RightOperandOpt)


consider changing to if (rangeExpr.RightOperandOpt is null) ... else ...

RikkiGibson · 2019-06-05T00:06:42Z

src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs

@@ -384,44 +363,29 @@ .locals init (System.Span<int> V_0, //s
  IL_0056:  stloc.s    V_4
  IL_0058:  ldloca.s   V_4


ldloca.s V_4 [](start = 12, length = 14)

Is this temp here to avoid bad behavior from repeated reads of the receiver? Just trying to picture what scenarios would cause trouble if the original were used instead of a temp.

Right, if the receiver is a property or method call (something with side effects) we don't want to evaluate it multiple times, so we create a temp.

RikkiGibson

agocke · 2019-06-06T19:36:43Z

@dotnet/roslyn-compiler for one more review please

jcouv · 2019-06-07T18:00:11Z

Looking #Closed

jcouv · 2019-06-08T00:11:22Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs

-            // int end = range.End.GetOffset(length)
-            // receiver.Slice(start, end - start)
+            // int rangeSize = range.End.GetOffset(length) - start
+            // receiver.Slice(start, rangeSize)


receiver.Slice(start, rangeSize) [](start = 14, length = 33)

Is there a design doc for ranges, where we can document the API patterns recognized by the compiler? #Closed

https://github.com/dotnet/csharplang/blob/8152182f0a477cb3082e625b607262cc459a17f3/proposals/index-range-changes.md #Resolved

jcouv · 2019-06-08T00:12:31Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

+                            // during lowering, but it's simpler to always require them to prevent
+                            // the user from getting surprising errors when optimizations fail
+                            _ = GetWellKnownTypeMember(Compilation, WellKnownMember.System_Range__get_Start, diagnostics, syntax: syntax);
+                            _ = GetWellKnownTypeMember(Compilation, WellKnownMember.System_Range__get_End, diagnostics, syntax: syntax);


GetWellKnownTypeMember [](start = 32, length = 22)

consider factoring the GetWellKnownTypeMember checks into a local function and calling here and above. #Closed

jcouv · 2019-06-08T00:24:48Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs

+
+                bool usedLength = false;
+
+                if (rangeExpr.LeftOperandOpt is null)


rangeExpr.LeftOperandOpt is null [](start = 20, length = 32)

nit: Consider inverting if and use a pattern like below: if (rangeExpr.RightOperandOpt is BoundExpression left) .... Or invert the if below as Rikki already pointed out ;-) #Closed

jcouv · 2019-06-08T00:30:11Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs


            var F = _factory;

+            var localsBuilder = ArrayBuilder<LocalSymbol>.GetInstance();


.GetInstance(); [](start = 57, length = 15)

On line 221, you use an optimistic size (3), but here and on line below you don't pass any size. Is that worth aligning? #Closed

I'm making a very subtle guess here. For Index, there's a pattern optimization where we don't need the Length if the Index is from the start, being created inline via a conversion. So the code would have to look like:

int offset = ...; int item = collection[(Index)offset];

Note that this won't work if the offset is either from the end, or if there's no Index conversion (since the normal behavior is to call the indexer that takes an int directly).

So my prediction is that almost every pattern Index indexer optimization will be from the end. Thus, I hint that explicitly.

For Range, it's completely different. Code like 0..5 seems like it will very common, and because Range only takes Indexes, the construction requires the use of the Index -> int explicit conversion, that I try to remove optimistically. So in this case, if the user is making a range only from start, or only from end, they won't or will require the length, respectively.

Here, I have no idea which will be more common, so I don't hint anything.

Do you think this is worth a comment? #Closed

Fine to leave as-is. Thanks

In reply to: 291857095 [](ancestors = 291857095)

jcouv · 2019-06-08T00:30:19Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs

+
+                if (usedLength)
+                {
+                    localsBuilder.Insert(1, lengthLocal.LocalSymbol);


Insert(1, [](start = 34, length = 10)

why not .Add(?
(also on next line) #Closed

I believe side effects for the synthesized nodes are evaluated in order, so if we need the length, we need it before the bound nodes that use it, but after receiver, which is the first side effect calculated. #Closed

jcouv · 2019-06-08T00:36:00Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs

+
+            BoundExpression startExpr;
+            BoundExpression rangeSizeExpr;
+            if (rangeArg is BoundRangeExpression rangeExpr)


if [](start = 12, length = 2)

The test cases I would expect for the new/optimized code: pattern indexer with .., ..y, x.. and x..y.
I wasn't able to identify test(s) for all of them. Can you confirm the coverage? #Closed

I looked through and found a test for everything except ... I've added one in the latest commit. #Closed

jcouv

Done with review pass (iteration 5)

agocke · 2019-06-11T02:20:05Z

@jcouv Anything else?

agocke · 2019-06-11T23:47:12Z

ping @jcouv

jcouv

LGTM Thanks (iteration 7)

agocke added 4 commits June 3, 2019 23:17

Add pattern Index access with explicit conversion to test

7cd53a7

Optimize indexer access of an inline range expression

e423d10

Introduces optimizations in lowering for an indexer access of the form receiver[a..b]. With the optimizations, the generation of intermediate System.Index and System.Range types will be avoided.

Add WellKnownMembers for test

c9fcba9

Add test fixup

e96c095

agocke changed the title ~~Optimize pattern range~~ Optimize indexer access of an inline range expression Jun 4, 2019

agocke marked this pull request as ready for review June 4, 2019 19:58

agocke requested a review from a team as a code owner June 4, 2019 19:58

agocke added the Area-Compilers label Jun 4, 2019

jcouv added the Feature - Range Range label Jun 4, 2019

jcouv added this to the 16.2.P3 milestone Jun 4, 2019

RikkiGibson reviewed Jun 4, 2019

View reviewed changes

RikkiGibson reviewed Jun 5, 2019

View reviewed changes

RikkiGibson approved these changes Jun 5, 2019

View reviewed changes

Simplify some code

8a21607

jcouv reviewed Jun 8, 2019

View reviewed changes

jinujoseph modified the milestones: 16.2.P3, 16.2 Jun 9, 2019

agocke added 2 commits June 10, 2019 11:33

Respond to PR Comments

4f77a32

Also refactor well known member checks into local function

2dd886e

jcouv approved these changes Jun 12, 2019

View reviewed changes

agocke merged commit ac17b9c into dotnet:master Jun 12, 2019

agocke deleted the optimize-pattern-range branch June 12, 2019 21:53

		@@ -384,44 +363,29 @@ .locals init (System.Span<int> V_0, //s
		IL_0056: stloc.s V_4
		IL_0058: ldloca.s V_4


		bool usedLength = false;

		if (rangeExpr.LeftOperandOpt is null)


		var F = _factory;

		var localsBuilder = ArrayBuilder<LocalSymbol>.GetInstance();

Conversation

agocke commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

agocke Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

agocke commented Jun 6, 2019

Uh oh!

jcouv commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke Jun 10, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke Jun 10, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke Jun 10, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agocke Jun 10, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

agocke commented Jun 11, 2019

Uh oh!

agocke commented Jun 11, 2019

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

agocke commented Jun 4, 2019 •

edited

Loading

jcouv commented Jun 7, 2019 •

edited

Loading

jcouv Jun 8, 2019 •

edited

Loading

agocke Jun 10, 2019 •

edited by jcouv

Loading

jcouv Jun 8, 2019 •

edited

Loading

jcouv Jun 8, 2019 •

edited

Loading

jcouv Jun 8, 2019 •

edited

Loading

agocke Jun 10, 2019 •

edited by jcouv

Loading

jcouv Jun 8, 2019 •

edited

Loading

agocke Jun 10, 2019 •

edited by jcouv

Loading

jcouv Jun 8, 2019 •

edited

Loading

agocke Jun 10, 2019 •

edited by jcouv

Loading