Discussion: "out this" parameters

[alrz]

Currently, discards always pass a local to the Deconsturct method (except for well-known ValueTuple in which case we use element fields directly),

var (x, _) = e;

// ->

e.Deconstruct(out var x, out var _);

With out this parameters we can keep the target member in metadata and use it instead, removing discarded locals.

// assuming either of these
void Deconstruct(out this.Foo, out this.Bar) {}
void Deconstruct(out string foo = this.Foo, out string bar = this.Bar) {}

// ...

var (x, _) = e;

// ->

var x = e.Foo;

This should also work for extension methods,

static void Deconstruct<T, U>(this KeyValuePair<T, U> @this, out this.Key, out this.Value) {}

var (key, _) = e;

// ->

var key = e.Key;

Note: Assignments should still be emitted inside the method in case we're using an older compiler.

This relates to #277 and will be more significant with recursive patterns in general. Once we have those concepts, the number of locals will increase under the hood.

Update: We can require all of the parameters to be "out this" and remove the body to avoid potential side effects, since we're skipping the call to Deconstruct altogether.

public void Deconstruct(out this.FirstName, out this.LastName);

--
Open question: Should this be only permitted for Deconstruct methods?

Open question: Should this be only permitted for out parameters?

public Foo WithName(this.Name) => this;

// ->

public Foo WithName(string name) { 
  this.Name = name;
  return this;
}

Open question: Should this be only permitted for this values?

void M(out int p = 0) {
  // `p` is always definitely assigned 
}

Note: this is not to be confused with "caller-receiver parameters".

[svick]

What's the point? Is it to save stack space for the unused variables? To avoid unnecessarily loading the fields/properties? Aren't both of those optimizations that the JIT could do?

[alrz]

@svick

This does not just avoid the unnecessary locals, bound variables are also directly assigned.

To avoid unnecessarily loading the fields/properties?

Does "loading" properties actually make sense here? unused property getters just won't be called.

[svick]

This does not just avoid the unnecessary locals, bound variables are also directly assigned.

You mean that it skips calling Deconstruct()? Sure, but that's pretty-much zero-cost, as long as the method call is inlined.

Does "loading" properties actually make sense here? unused property getters just won't be called.

Right, I was assuming that getters are trivial (read: can be inlined and optimized away if unused).

---

My point is: I think those are all optimizations that the JIT could do with no modification to the language. If you think this is a performance problem, maybe asking for the JIT to optimize this kind of code better would make more sense thank asking for the language to change?

(The one exception are non-trivial getters, which the JIT probably won't be able to completely optimize away anytime soon. I'm not sure that's enough to justify this feature.)

[alrz]

While I agree that this is just to "optimize" the generated code aside form some syntax candy, I can no longer buy the assumptions about things JIT hopefully probably should do. Is there any way to actually test if it does the inlining?

[bondsbw]

Might as well get rid of the block requirement too:

void Deconstruct(out this.Foo, out this.Bar);

(Maybe just in the case of all parameters being out this.)

[mattwar]

With the "out this" Deconstruct method, would the method call be omitted in favor or property access always, or is this only for when some of the desconstruction is being discarded?

[alrz]

@mattwar

It's always omitted (assuming that's ok wrt side effects), because with "out this" parameters, we always know that which position corresponds to which member. Perhaps we should require "all or none" of parameters to be out this and eventually get rid of the block as @bondsbw suggested, to make sideeffects impossible. I'll note that in addition to sideeffects in Deconstruct method (which will be avoided due to non-existent), sideeffects in discarded properties is something we should avoid, even JIT inlining will not help with that.

[svick]

@alrz

While I agree that this is just to "optimize" the generated code aside form some syntax candy, I can no longer buy the assumptions about things JIT hopefully probably should do.

There are two options here:

1. Improve the JIT optimizer to handle this case better. This is probably going to be a relatively small change to one class in the CLR.
2. Design and implement a whole new language feature. This includes changes to pretty much all stages of the compiler. Together, it's going to be a fairly big change.

I don't see how would option 2 be worth it, even if you could make it happen sooner than option 1.

Is there any way to actually test if it does the inlining?

The simple option is to use SharpLab, which now also supports decompiling. Though I believe it uses .Net Framework x86 JIT, so the output is not representative of the state of the art. (It also currently doesn't seem to support tuples and deconstruction, but that's probably temporary.)

The right option is to follow this guide to build CoreCLR from source and use it to output the assembly.

Using that approach and this code, the results are:

; var (x, _) = e;
mov      eax, dword ptr [rcx]
mov      rax, gword ptr [rcx+8]

; var x = e.Foo;
mov      rax, gword ptr [rcx+8]

So, the Deconstruct method and trivial property getters are completely inlined and Bar (which would be rcx+16) is not loaded. I believe the extra mov is null reference check and I don't understand why it's not eliminated.

To sum up: After looking at the generated assembly, I'm even more convinced this does not make sense. In the case of trivial property getters (which I think are the common case for Deconstruct), this would only improve the assembly due to a quirk in the current JIT compiler.

[alrz]

@svick

Ok, just one question. How would JIT compiler know that I'm not interested in sideeffects of get_Bar? I agree that deconstruction is defined as calling Deconstruct, but with discards I've made it clear that I'm not interested in that particular value and I'd prefer to avoid those sideeffects.

[svick]

@alrz

How would JIT compiler know that I'm not interested in sideeffects of get_Bar?

It wouldn't. But I think that having side-effects in Deconstruct and not wanting them some of the time is going to be to be fairly rare and probably not enough to justify this feature.

[alrz]

I'm not talking about Deconstruct method itself. There's a higher chance for Bar getter to have sideeffects. Like caching the result the first time it's being read.

[svick]

@alrz Yeah, I meant, Deconstruct having side-effects, or any method called from Deconstruct (like the Bar getter) having them.

[alrz]

@svick

So it wouldn't be rare. I'm trying to say that the only reason you want to define a Deconstruct method is to give an "order" to members to be able to use deconstruction and later, positional patterns. If it comes at the cost of evaluating every member at the time of use (in presence of discards), I'd say that a discard is not what it meant to be.

[bondsbw]

If side effects of all properties need to be evaluated at the same time (as opposed to only the side effects of properties which are not discarded), then the standard Deconstruct could be used without out this.

Or really the properties should be written to force evaluation of each other. It is likely bad design to force the caller to be aware of such a constraint, especially if the properties are directly visible outside the class.

[gafter]

@svick "this" parameter defaults are already being considered as a way of making "With" methods work. This proposal is a small increment to that.