theolivenbaum commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696942135


   @jeme interesting benchmark - if I'm reading right, seems like the only difference is in case there are a lot of nulls being passed
   
   | Method | Job | Runtime | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
   | ------------- |:-:|:-:|:-:|:-:|:-:|:-:|:-:|:-:|:-:|
   | IsPatternMatching | .NET Core 3.1 | .NET Core 3.1 | 1.3430 ns | 0.0292 ns | 0.0273 ns | - | - | - | -| 
   | NotNullIsPatternMatching | .NET Core 3.1 | .NET Core 3.1 | 1.5368 ns | 0.0275 ns | 0.0257 ns | - | - | - | -| 
   | NullIsPatternMatching | .NET Core 3.1 | .NET Core 3.1 | 0.2990 ns | 0.0214 ns | 0.0190 ns | - | - | - | -| 
   | NullIsPatternNonMatching | .NET Core 3.1 | .NET Core 3.1 | 0.2634 ns | 0.0258 ns | 0.0242 ns | - | - | - | -|
   
   
   Think you missed only one test:
   
   ```csharp
           [Benchmark]
           public int IsPatternMatchingButNull()
           {
               if (nullitem is ArrayList table) return table.Count;
               return 0;
           }
   ```
   
   I've added back the `!(a is null) && a is Class c` checks for now: https://github.com/apache/lucenenet/pull/345/commits/d08b443d14d751241b2951b18bd4a0f9a918b92a
   
   but I think it's worth cleaning up most of them back to just the pattern matching, as I doubt there is going to be any impact on 99% of the cases...
   
   I'll try tomorrow to setup testing on our Azure DevOps to be sure I'm not breaking anything with these changes.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org