Question

So I came across this little gem in our codebase the other day, and I wanted to try and see if the person who wrote it was just lazy, or knew something that I don't.

A standard event handler was written like this (I will -

private void OnSomeEvent(IVehicle sender, ISomeArgs args){

 if((sender is Car) && (sender as Car).numWheels == 4 && (sender as Car).hasGas) 
 {
     (sender as Car).drive();
 }

}

I saw this an immediately thought of the numerous un-boxing type-casting operations that are needlessly being done here. I re-wrote it like so-

private void OnSomeEvent(IVehicle sender, ISomeArgs args){


 if (sender is Car){
  Car _car = sender as Car;

   if(_car.numWheels == 4 && _car.hasGas){
     _car.drive();
   }
 }     


}

So did the first example know something I don't? Does the compiler know that we are trying to un-box type-cast to the same type a bunch of times and do some optimization?

Was it helpful?

Solution

You've already accepted an answer, but I've already done the looking so I figure I'll post it.

Checking the output IL when doing a Release build, there's very little difference between the two setups.

The output IL from the first "unoptimized" call looks like so:

// Code size       47 (0x2f)
.maxstack  8
IL_0000:  ldarg.0
IL_0001:  isinst     TestApp.Program/Car
IL_0006:  brfalse.s  IL_002e
IL_0008:  ldarg.0
IL_0009:  isinst     TestApp.Program/Car
IL_000e:  callvirt   instance int32 TestApp.Program/Car::get_numWheels()
IL_0013:  ldc.i4.4
IL_0014:  bne.un.s   IL_002e
IL_0016:  ldarg.0
IL_0017:  isinst     TestApp.Program/Car
IL_001c:  callvirt   instance bool TestApp.Program/Car::get_hasGas()
IL_0021:  brfalse.s  IL_002e
IL_0023:  ldarg.0
IL_0024:  isinst     TestApp.Program/Car
IL_0029:  callvirt   instance void TestApp.Program/Car::drive()
IL_002e:  ret

The output IL from the second "optimized" call looks like:

// Code size       34 (0x22)
.maxstack  2
.locals init ([0] class TestApp.Program/Car c)
IL_0000:  ldarg.0
IL_0001:  isinst     TestApp.Program/Car
IL_0006:  stloc.0
IL_0007:  ldloc.0
IL_0008:  brfalse.s  IL_0021
IL_000a:  ldloc.0
IL_000b:  callvirt   instance int32 TestApp.Program/Car::get_numWheels()
IL_0010:  ldc.i4.4
IL_0011:  bne.un.s   IL_0021
IL_0013:  ldloc.0
IL_0014:  callvirt   instance bool TestApp.Program/Car::get_hasGas()
IL_0019:  brfalse.s  IL_0021
IL_001b:  ldloc.0
IL_001c:  callvirt   instance void TestApp.Program/Car::drive()
IL_0021:  ret

There are additional isinst calls being made in the "unoptimized" call, which are good to be optimized away, but won't substantially impact the runtime of the function.

That said, I'd still do the "optimization" as the C# code is easier to read, which is more important than any performance micro-optimizations (until you profile your code and determine that you need to optimize a performance bottleneck).

(Also, the stack is slightly larger, but since it's just a stack increase which is fast and immediately cleaned up, and it's only 6 bytes, it's extremely minor.)

OTHER TIPS

You don't need is when you use as because as will return the actual object of desired type if it's castable to that type, otherwise the null value is returned. Just checking if it's null would be the most optimized way:

private void OnSomeEvent(IVehicle sender, ISomeArgs args){    
   Car _car = sender as Car;
   if(_car != null){
    if(_car.numWheels == 4 && _car.hasGas){
     _car.drive();
    }
   }     
}

Boxing and unboxing are done only between value types and reference types.

From msdn:

Unboxing is an explicit conversion from the type object to a value type or from an interface type to a value type that implements the interface.

So there isn't any unboxing in your code. There is only a series of type casting that does not affect performances noticeably.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top