Roslyn: Type pattern is less efficient for value types than is/cast

[jnm2]

If you use a type pattern, it creates an unnecessary local with an unnecessary load and store which are not optimized away.

return obj is SomeValueType someValueType && Equals(someValueType);

Desugars to:

object obj2;
if ((obj2 = obj) is SomeValueType)
{
    SomeValueType other = (SomeValueType)obj2;
    return Equals(other);
}
return false;

It generates an extra four assembly instructions.

SharpLab asm: type pattern vs is/cast
SharpLab IL: type pattern vs is/cast

/cc @gafter, @CyrusNajmabadi

[jnm2]

Is this intentional, on the off chance that someone took a ref to the parameter and is mutating it from a background thread? Wouldn't happen with a parameter, but perhaps a field. That scenario would be prone to race conditions either way—is that even worth considering?

[CyrusNajmabadi]

That scenario would be race-condition-prone either way—is that even worth considering?

it's def worth considering given that hte language would not want you to be able to observe such a thing. However, that said, we could consider some sort of optimization (or better codegen) when we can see that that local/parameter is never used in such a fashion. In that case, the copy to the temp could be considered unnecessary and could optimize away. @gafter for his thoughts on how viable any sort of approach is here.

[jnm2]

Wasn't thinking at first, but that scenario could result in InvalidCastException from the type pattern. Definitely bad. However, also not possible when the expression is a parameter or local. A ref to something on the stack can't be mutated concurrently with a type pattern's execution, right?

[CyrusNajmabadi]

Good point. I would hope not. Perhaps in the active-pattern world. However, that would be a different test than the basic "type test" we're talking about here. It does seems as if the temp for a local/parameter should not be necessary and shoudl be removable. That would make the compiler generate the same code for is T t as it does for the idiomatic code users write (i.e. x is T; (T)x). And that would be a great thing so that there's no downside to moving to the pattern approach.

[Neme12]

I don't see what the concern is here. Are race conditions well defined behavior in the language (as opposed to something like C++)?

[jnm2]

@Neme12 I would expect that obj is Foo foo should either fail or succeed but never throw InvalidCastException. I think it should protect against that. However, no need for that protection if foo is a local or parameter, for starters.

[Neme12]

I thought race conditions were always UB...

I would expect that obj is Foo foo should either fail or succeed but never throw InvalidCastException.

that's sounds to me like saying "I would expect a++ to always fail or succeed" or "I would expect a = b; to always copy the all the data or nothing". If you have a race condition, InvalidCastException is probably one of the better outcomes.

[CyrusNajmabadi]

@Neme12 These are for locals/parameters. He's saying: there should be no race conditions here. So the compiler is being overly zealous in producing a temp. it should avoid the temp and just do the work on the actual local/parameter as there's no way for other code to come in and affect the variable in an observable manner (including in multi-threaded environments).

[jnm2]

If you have a race condition, InvalidCastException is probably one of the better outcomes.

I disagree. This is more similar to ?.Invoke than anything else.

[Neme12]

I disagree. This is more similar to ?.Invoke than anything else.

That is only the case for properties (which can really return anything at any time), not fields. If you're reading from a field and someone else is writing to it concurrently, you already have a problem.

My question is, why can't this optimization be done for fields as well?

[CyrusNajmabadi]

My question is, why can't this optimization be done for fields as well?

@Neme12 It's unclear what you're referring to. Do you mean not having the compiler write into a temporary for doing something like this.x is Z z?

[Neme12]

yes

[CyrusNajmabadi]

If you're reading from a field and someone else is writing to it concurrently, you already have a problem.

This is not true. Lots of code can (and is) written with this working properly. You have to be careful, but the language has defined semantics for what's going on.

Effectively, if you are doing anything defined as the language as multiple accesses, then all bets are off. So, for example, "a++" is a read and a write to a variable. As those are two separate things, you have no guarantees about what may happen.

however, just accessing a variable is not undefined behavior in C#. So code that just reads is safe. And that means the compiler needs to properly generate IL that produces only a single actual read to prevent problems.

[Neme12]

however, just accessing a variable is not undefined behavior in C#

How can that be true for arbitrarily sized data types?

[Neme12]

So code that just reads is safe.

Yes. I was talking about code that reads and writes at the same time.

[CyrusNajmabadi]

Yes. I was talking about code that reads and writes at the same time.

There is no reason that has to be a problem. And there is lots of code that will work even in the presence of that. Again, you have to be careful, and know what you're doing.

[Neme12]

Ok sorry, I didn't know that C# guaranteed this.

[CyrusNajmabadi]

Also, calling this an optimization is a bit of a misnomer. it's normally a good thing to actually store this to a local. Consider if this is a field, if you have to read from it multiple times that might actually be quite expensive in practice. Storing into the local and just using that means avoiding any sort of memory lookups.

[CyrusNajmabadi]

The issue here is that storing into a temp (while good in general, and necessary for correctness in some circumstances) seems unnecessary when the original variable is a local or a parameter.

[jnm2]

@Neme12 One of the key value propositions of ?.Invoke was that we'd no longer have to make a manual defensive copy in case someone unsubscribed from the event from another thread.

As further evidence, see the compare-and-swap loop the compiler generates every time you declare an auto event:
https://sharplab.io/#v2:EYLgHgbALANALiAlgGwD4AEAMACdBGAbgFgAoU9AZlwCZsBhUgb1O1dyoFMA3DgOzmwBRHvwASAQ14ATZBwBO2AMoB7ALYdhfOMRIBfIA===

[gafter]

This is fixed in the pending C# 8 implementation. See HERE.