You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by GitBox <gi...@apache.org> on 2020/09/22 13:26:29 UTC
[GitHub] [lucenenet] theolivenbaum opened a new pull request #345: Reduce casting
theolivenbaum opened a new pull request #345:
URL: https://github.com/apache/lucenenet/pull/345
Hi @NightOwl888 - did a first pass on some applying some analyzer changes for a few quick wins:
- change ` (a != null && a is MyClass)` to `a is MyClass mc`
- change `== null` to `is null`
- change `!= null` to `is object`
I did double check that there was no crazy operator overloading for == that would change behavior - it seems like only the [State.cs](https://github.com/apache/lucenenet/blob/master/src/Lucene.Net/Util/Automaton/State.cs#L377) class has overloaded it, and the behavior is the same with is null / is object.
Also changed a few fields to read-only, and one regex variable to static as suggested by the analyzer too.
Regarding your previous comment, do you think this kind of changes needs the "LUCENENET" comments you mentioned?
By the way, is there any docs or quick start on running the tests locally? And are the tests run automatically on PR, or manually? Just to know if I should bother trying to set it up locally, or easier to let them run and just check the results...
----------------------------------------------------------------
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
[GitHub] [lucenenet] theolivenbaum commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
theolivenbaum commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696835930
@NightOwl888 just checking on [sharplab again](https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQ64GYAEBLAO2AFMAnAMzAGNicBJAWQE86izKacBvAX3U1yIczAMIAbSBBwh6zViQrVaXHHwzrsOOEgBsWhDgAqxCMG7oclnBaubteuABYcAQQAUjFm0WcqASnM0K2D8cjcqHABCAF4cAgBXMTEcADIUnAi8KVEJCCkqf0CQ4q0kAE5wqgA6AHFiYEMmAAdiNz8/AG4bYrVg3qturUFdLWcAIQ85bw5aQq5B4LwwiJi4xLEA+aCS4IA3MFIcKDAmACNaWIjIYSZxSS7tncslt2Oz2lWEpL8tp5LtCpvc61eqNFptToLEr9HqDGHWR5DUoOZwiSZeBQzDKbKHPZb4bK3XL5KgIHGInYAyoIEENZqtdoPHbwnjBQaDOwjJw4AAi6Pk7CU2KKJRemSkAHtTgArYhUMxpDIEm53PIZUnkv6WKkFGl1Ongxm41Rw9A8IA). It seems both these options produce the exact same IL/ASM:
```
public static void A(IMyInterface c) {
if(c != null && c is MyClass cc) {
Console.WriteLine(cc.GetType());
}
}
```
```
public static void D(IMyInterface c) {
if(c is object && c is MyClass cc2) {
Console.WriteLine(cc2.GetType());
}
}
```
Do you prefer I undo the pattern matching changes, or add the extra `is object` to them so moving forward you also don't get the warning on VS?
----------------------------------------------------------------
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
[GitHub] [lucenenet] jeme edited a comment on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
jeme edited a comment on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696880214
Been trying to put some benchmarks together, can't quite get the Allocations into the report, but so far there seems to be no reasons to avoid pattern matching...
Also, this doesn't quite hit all the cases (e.g. where there are null checks before the conversions etc.) so will work a bit further to see if I can get that in and have the report still be digestable.
``` ini
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18362.1082 (1903/May2019Update/19H1)
Intel Core i9-7900X CPU 3.30GHz (Kaby Lake), 1 CPU, 20 logical and 10 physical cores
.NET Core SDK=3.1.301
[Host] : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
.NET 4.8 : .NET Framework 4.8 (4.8.4220.0), X64 RyuJIT
.NET Core 3.1 : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
```
| Method | Job | Runtime | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------- |-------------- |-------------- |---------:|----------:|----------:|------:|------:|------:|----------:|
| AsOperatorNullCheckMatching | .NET 4.8 | .NET 4.8 | 1.748 ns | 0.0368 ns | 0.0344 ns | - | - | - | - |
| AsOperatorNullCheckNonMatching | .NET 4.8 | .NET 4.8 | 2.094 ns | 0.0708 ns | 0.0757 ns | - | - | - | - |
| IsPatternMatching | .NET 4.8 | .NET 4.8 | 1.812 ns | 0.0577 ns | 0.0539 ns | - | - | - | - |
| IsPatternNonMatching | .NET 4.8 | .NET 4.8 | 2.105 ns | 0.0321 ns | 0.0284 ns | - | - | - | - |
| IsThenCastMatching | .NET 4.8 | .NET 4.8 | 2.117 ns | 0.0706 ns | 0.0660 ns | - | - | - | - |
| IsThenCastNonMatching | .NET 4.8 | .NET 4.8 | 2.512 ns | 0.0760 ns | 0.0781 ns | - | - | - | - |
| AsOperatorNullCheckMatching | .NET Core 3.1 | .NET Core 3.1 | 1.478 ns | 0.0297 ns | 0.0263 ns | - | - | - | - |
| AsOperatorNullCheckNonMatching | .NET Core 3.1 | .NET Core 3.1 | 1.843 ns | 0.0431 ns | 0.0382 ns | - | - | - | - |
| IsPatternMatching | .NET Core 3.1 | .NET Core 3.1 | 1.543 ns | 0.0448 ns | 0.0419 ns | - | - | - | - |
| IsPatternNonMatching | .NET Core 3.1 | .NET Core 3.1 | 1.890 ns | 0.0531 ns | 0.0443 ns | - | - | - | - |
| IsThenCastMatching | .NET Core 3.1 | .NET Core 3.1 | 1.898 ns | 0.0603 ns | 0.0884 ns | - | - | - | - |
| IsThenCastNonMatching | .NET Core 3.1 | .NET Core 3.1 | 2.294 ns | 0.0525 ns | 0.0438 ns | - | - | - | - |
----
```csharp
[MemoryDiagnoser]
[SimpleJob(RuntimeMoniker.Net48)]
[SimpleJob(RuntimeMoniker.NetCoreApp31)]
public class PatternMatching
{
private object item = new ArrayList();
private object nullitem = null;
[Benchmark]
public int AsOperatorNullCheckMatching()
{
var array = item as ArrayList;
if (array != null) return array.Count;
return 0;
}
[Benchmark]
public int AsOperatorNullCheckNonMatching()
{
var table = item as Hashtable;
if (table != null) return table.Count;
return 0;
}
[Benchmark]
public int IsPatternMatching()
{
if (item is ArrayList table) return table.Count;
return 0;
}
[Benchmark]
public int IsPatternNonMatching()
{
if (item is Hashtable table) return table.Count;
return 0;
}
[Benchmark]
public int IsThenCastMatching()
{
if (item is ArrayList) return ((ArrayList)item).Count;
return 0;
}
[Benchmark]
public int IsThenCastNonMatching()
{
if (item is Hashtable) return ((Hashtable)item as Hashtable).Count;
return 0;
}
}
```
----------------------------------------------------------------
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
[GitHub] [lucenenet] eladmarg commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
eladmarg commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696722983
----------------------------------------------------------------
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
[GitHub] [lucenenet] jeme commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
jeme commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696897183
Adding a null check before the pattern matching takes place:
----
``` ini
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18362.1082 (1903/May2019Update/19H1)
Intel Core i9-7900X CPU 3.30GHz (Kaby Lake), 1 CPU, 20 logical and 10 physical cores
.NET Core SDK=3.1.402
[Host] : .NET Core 3.1.8 (CoreCLR 4.700.20.41105, CoreFX 4.700.20.41903), X64 RyuJIT
.NET 4.8 : .NET Framework 4.8 (4.8.4220.0), X64 RyuJIT
.NET Core 3.1 : .NET Core 3.1.8 (CoreCLR 4.700.20.41105, CoreFX 4.700.20.41903), X64 RyuJIT
```
| Method | Job | Runtime | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---------------------------- |-------------- |-------------- |----------:|----------:|----------:|------:|------:|------:|----------:|
| IsPatternMatching | .NET 4.8 | .NET 4.8 | 1.8286 ns | 0.0508 ns | 0.0451 ns | - | - | - | - |
| NotNullIsPatternMatching | .NET 4.8 | .NET 4.8 | 1.7808 ns | 0.0520 ns | 0.0486 ns | - | - | - | - |
| NullIsPatternMatching | .NET 4.8 | .NET 4.8 | 0.2407 ns | 0.0251 ns | 0.0235 ns | - | - | - | - |
| IsPatternNonMatching | .NET 4.8 | .NET 4.8 | 2.0952 ns | 0.0367 ns | 0.0306 ns | - | - | - | - |
| NotNullIsPatternNonMatching | .NET 4.8 | .NET 4.8 | 2.1074 ns | 0.0341 ns | 0.0302 ns | - | - | - | - |
| NullIsPatternNonMatching | .NET 4.8 | .NET 4.8 | 0.2694 ns | 0.0063 ns | 0.0056 ns | - | - | - | - |
| 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 | - | - | - | - |
| IsPatternNonMatching | .NET Core 3.1 | .NET Core 3.1 | 1.9393 ns | 0.0542 ns | 0.0507 ns | - | - | - | - |
| NotNullIsPatternNonMatching | .NET Core 3.1 | .NET Core 3.1 | 1.8874 ns | 0.0295 ns | 0.0261 ns | - | - | - | - |
| NullIsPatternNonMatching | .NET Core 3.1 | .NET Core 3.1 | 0.2634 ns | 0.0258 ns | 0.0242 ns | - | - | - | - |
----
```csharp
[Benchmark]
public int IsPatternMatching()
{
if (item is ArrayList table) return table.Count;
return 0;
}
[Benchmark]
public int NotNullIsPatternMatching()
{
if (item != null && item is ArrayList table) return table.Count;
return 0;
}
[Benchmark]
public int NullIsPatternMatching()
{
if (nullitem != null && nullitem is ArrayList table) return table.Count;
return 0;
}
[Benchmark]
public int IsPatternNonMatching()
{
if (item is Hashtable table) return table.Count;
return 0;
}
[Benchmark]
public int NotNullIsPatternNonMatching()
{
if (item != null && item is Hashtable table) return table.Count;
return 0;
}
[Benchmark]
public int NullIsPatternNonMatching()
{
if (nullitem != null && nullitem is Hashtable table) return table.Count;
return 0;
}
```
----------------------------------------------------------------
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
[GitHub] [lucenenet] theolivenbaum commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
theolivenbaum commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-697205762
I'll close this PR and send separate ones for != null and == null to keep things more manageable
----------------------------------------------------------------
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
[GitHub] [lucenenet] NightOwl888 commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696770379
----------------------------------------------------------------
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
[GitHub] [lucenenet] jeme edited a comment on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
jeme edited a comment on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696880214
Been trying to put some benchmarks together, can't quite get the Allocations into the report, but so far there seems to be no reasons to avoid pattern matching...
Also, this doesn't quite hit all the cases (e.g. where there are null checks before the conversions etc.) so will work a bit further to see if I can get that in and have the report still be digestable.
``` ini
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18362.1082 (1903/May2019Update/19H1)
Intel Core i9-7900X CPU 3.30GHz (Kaby Lake), 1 CPU, 20 logical and 10 physical cores
.NET Core SDK=3.1.301
[Host] : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
.NET 4.8 : .NET Framework 4.8 (4.8.4220.0), X64 RyuJIT
.NET Core 3.1 : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
```
| Method | Job | Runtime | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------- |-------------- |-------------- |---------:|----------:|----------:|------:|------:|------:|----------:|
| AsOperatorNullCheckMatching | .NET 4.8 | .NET 4.8 | 1.748 ns | 0.0368 ns | 0.0344 ns | - | - | - | - |
| AsOperatorNullCheckNonMatching | .NET 4.8 | .NET 4.8 | 2.094 ns | 0.0708 ns | 0.0757 ns | - | - | - | - |
| IsPatternMatching | .NET 4.8 | .NET 4.8 | 1.812 ns | 0.0577 ns | 0.0539 ns | - | - | - | - |
| IsPatternNonMatching | .NET 4.8 | .NET 4.8 | 2.105 ns | 0.0321 ns | 0.0284 ns | - | - | - | - |
| IsThenCastMatching | .NET 4.8 | .NET 4.8 | 2.117 ns | 0.0706 ns | 0.0660 ns | - | - | - | - |
| IsThenCastNonMatching | .NET 4.8 | .NET 4.8 | 2.512 ns | 0.0760 ns | 0.0781 ns | - | - | - | - |
| AsOperatorNullCheckMatching | .NET Core 3.1 | .NET Core 3.1 | 1.478 ns | 0.0297 ns | 0.0263 ns | - | - | - | - |
| AsOperatorNullCheckNonMatching | .NET Core 3.1 | .NET Core 3.1 | 1.843 ns | 0.0431 ns | 0.0382 ns | - | - | - | - |
| IsPatternMatching | .NET Core 3.1 | .NET Core 3.1 | 1.543 ns | 0.0448 ns | 0.0419 ns | - | - | - | - |
| IsPatternNonMatching | .NET Core 3.1 | .NET Core 3.1 | 1.890 ns | 0.0531 ns | 0.0443 ns | - | - | - | - |
| IsThenCastMatching | .NET Core 3.1 | .NET Core 3.1 | 1.898 ns | 0.0603 ns | 0.0884 ns | - | - | - | - |
| IsThenCastNonMatching | .NET Core 3.1 | .NET Core 3.1 | 2.294 ns | 0.0525 ns | 0.0438 ns | - | - | - | - |
----------------------------------------------------------------
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
[GitHub] [lucenenet] eladmarg commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
eladmarg commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696868907
if we don't gain any performance, I prefer to stick with readability,
so acceptable by me is == null -> is null
a is MyClass mc (assuming performance are the same)
keep != null, when c# 9.0 is out, we'll replace all with is not null
----------------------------------------------------------------
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
[GitHub] [lucenenet] jeme edited a comment on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
jeme edited a comment on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696880214
Been trying to put some benchmarks together, can't quite get the Allocations into the report, but so far there seems to be no reasons to avoid pattern matching...
Also, this doesn't quite hit all the cases (e.g. where there are null checks before the conversions etc.) so will work a bit further to see if I can get that in and have the report still be digestable.
``` ini
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18362.1082 (1903/May2019Update/19H1)
Intel Core i9-7900X CPU 3.30GHz (Kaby Lake), 1 CPU, 20 logical and 10 physical cores
.NET Core SDK=3.1.301
[Host] : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
.NET 4.8 : .NET Framework 4.8 (4.8.4220.0), X64 RyuJIT
.NET Core 3.1 : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
```
| Method | Job | Runtime | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------- |-------------- |-------------- |---------:|----------:|----------:|------:|------:|------:|----------:|
| AsOperatorNullCheckMatching | .NET 4.8 | .NET 4.8 | 1.748 ns | 0.0368 ns | 0.0344 ns | - | - | - | - |
| AsOperatorNullCheckNonMatching | .NET 4.8 | .NET 4.8 | 2.094 ns | 0.0708 ns | 0.0757 ns | - | - | - | - |
| IsPatternMatching | .NET 4.8 | .NET 4.8 | 1.812 ns | 0.0577 ns | 0.0539 ns | - | - | - | - |
| IsPatternNonMatching | .NET 4.8 | .NET 4.8 | 2.105 ns | 0.0321 ns | 0.0284 ns | - | - | - | - |
| IsThenCastMatching | .NET 4.8 | .NET 4.8 | 2.117 ns | 0.0706 ns | 0.0660 ns | - | - | - | - |
| IsThenCastNonMatching | .NET 4.8 | .NET 4.8 | 2.512 ns | 0.0760 ns | 0.0781 ns | - | - | - | - |
| AsOperatorNullCheckMatching | .NET Core 3.1 | .NET Core 3.1 | 1.478 ns | 0.0297 ns | 0.0263 ns | - | - | - | - |
| AsOperatorNullCheckNonMatching | .NET Core 3.1 | .NET Core 3.1 | 1.843 ns | 0.0431 ns | 0.0382 ns | - | - | - | - |
| IsPatternMatching | .NET Core 3.1 | .NET Core 3.1 | 1.543 ns | 0.0448 ns | 0.0419 ns | - | - | - | - |
| IsPatternNonMatching | .NET Core 3.1 | .NET Core 3.1 | 1.890 ns | 0.0531 ns | 0.0443 ns | - | - | - | - |
| IsThenCastMatching | .NET Core 3.1 | .NET Core 3.1 | 1.898 ns | 0.0603 ns | 0.0884 ns | - | - | - | - |
| IsThenCastNonMatching | .NET Core 3.1 | .NET Core 3.1 | 2.294 ns | 0.0525 ns | 0.0438 ns | - | - | - | - |
----
```
[MemoryDiagnoser]
[SimpleJob(RuntimeMoniker.Net48)]
[SimpleJob(RuntimeMoniker.NetCoreApp31)]
public class PatternMatching
{
private object item = new ArrayList();
private object nullitem = null;
[Benchmark]
public int AsOperatorNullCheckMatching()
{
var array = item as ArrayList;
if (array != null) return array.Count;
return 0;
}
[Benchmark]
public int AsOperatorNullCheckNonMatching()
{
var table = item as Hashtable;
if (table != null) return table.Count;
return 0;
}
[Benchmark]
public int IsPatternMatching()
{
if (item is ArrayList table) return table.Count;
return 0;
}
[Benchmark]
public int IsPatternNonMatching()
{
if (item is Hashtable table) return table.Count;
return 0;
}
[Benchmark]
public int IsThenCastMatching()
{
if (item is ArrayList) return ((ArrayList)item).Count;
return 0;
}
[Benchmark]
public int IsThenCastNonMatching()
{
if (item is Hashtable) return ((Hashtable)item as Hashtable).Count;
return 0;
}
}
```
----------------------------------------------------------------
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
[GitHub] [lucenenet] NightOwl888 commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696870071
> keep != null, when c# 9.0 is out, we'll replace all with is not null
Agreed
----------------------------------------------------------------
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
[GitHub] [lucenenet] theolivenbaum commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
theolivenbaum commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696850081
@NightOwl888 yeah for sure different IL for the != case, but I did check if any operator overloads would cause issues with the changes.
My only minor issue with the `!(c is null)` option is that when the `is not null` finally becomes available, it will be harder to do a replace all :)
But happy to change to the `!(c is null)` case for now
----------------------------------------------------------------
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
[GitHub] [lucenenet] jeme commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
jeme commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696880214
Been trying to put some benchmarks together, can't quite get the Allocations into the report, but so far there seems to be no reasons to avoid pattern matching...
``` ini
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18362.1082 (1903/May2019Update/19H1)
Intel Core i9-7900X CPU 3.30GHz (Kaby Lake), 1 CPU, 20 logical and 10 physical cores
.NET Core SDK=3.1.301
[Host] : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
.NET 4.8 : .NET Framework 4.8 (4.8.4220.0), X64 RyuJIT
.NET Core 3.1 : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
```
| Method | Job | Runtime | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------- |-------------- |-------------- |---------:|----------:|----------:|------:|------:|------:|----------:|
| AsOperatorNullCheckMatching | .NET 4.8 | .NET 4.8 | 1.748 ns | 0.0368 ns | 0.0344 ns | - | - | - | - |
| AsOperatorNullCheckNonMatching | .NET 4.8 | .NET 4.8 | 2.094 ns | 0.0708 ns | 0.0757 ns | - | - | - | - |
| IsPatternMatching | .NET 4.8 | .NET 4.8 | 1.812 ns | 0.0577 ns | 0.0539 ns | - | - | - | - |
| IsPatternNonMatching | .NET 4.8 | .NET 4.8 | 2.105 ns | 0.0321 ns | 0.0284 ns | - | - | - | - |
| IsThenCastMatching | .NET 4.8 | .NET 4.8 | 2.117 ns | 0.0706 ns | 0.0660 ns | - | - | - | - |
| IsThenCastNonMatching | .NET 4.8 | .NET 4.8 | 2.512 ns | 0.0760 ns | 0.0781 ns | - | - | - | - |
| AsOperatorNullCheckMatching | .NET Core 3.1 | .NET Core 3.1 | 1.478 ns | 0.0297 ns | 0.0263 ns | - | - | - | - |
| AsOperatorNullCheckNonMatching | .NET Core 3.1 | .NET Core 3.1 | 1.843 ns | 0.0431 ns | 0.0382 ns | - | - | - | - |
| IsPatternMatching | .NET Core 3.1 | .NET Core 3.1 | 1.543 ns | 0.0448 ns | 0.0419 ns | - | - | - | - |
| IsPatternNonMatching | .NET Core 3.1 | .NET Core 3.1 | 1.890 ns | 0.0531 ns | 0.0443 ns | - | - | - | - |
| IsThenCastMatching | .NET Core 3.1 | .NET Core 3.1 | 1.898 ns | 0.0603 ns | 0.0884 ns | - | - | - | - |
| IsThenCastNonMatching | .NET Core 3.1 | .NET Core 3.1 | 2.294 ns | 0.0525 ns | 0.0438 ns | - | - | - | - |
----------------------------------------------------------------
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
[GitHub] [lucenenet] theolivenbaum commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
theolivenbaum commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696811978
----------------------------------------------------------------
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
[GitHub] [lucenenet] jeme commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
jeme commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696880214
----------------------------------------------------------------
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
[GitHub] [lucenenet] eladmarg commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
eladmarg commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696722983
Great PR! I'm happy with all changes except the pattern a is object instead a != null.
!= null is more readable for me, but its a matter of personal preferences.
----------------------------------------------------------------
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
[GitHub] [lucenenet] theolivenbaum closed pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
theolivenbaum closed pull request #345:
URL: https://github.com/apache/lucenenet/pull/345
----------------------------------------------------------------
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
[GitHub] [lucenenet] theolivenbaum commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
theolivenbaum commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696811978
@eladmarg : I also find it more readable (looking forward for the `is not null` from C#9, but is object I avoids having to call the the equality operator.
@NightOwl888 interesting point... From looking on the generated ASM:
https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQ64GYAEBLAO2AFMAnAMzAGNicBJAWQE86izKacBvAX3U1yIczAMIAbSBBwh6zViQrVaXHHwzrsOOEgBsWhDgAqxCMG7oclnBaubteuABYcAQQAUjFm0WcqASnM0K2D8cjcqHABCAF4cAgBXMTEcADIUnAi8KVEJCCkqf0CQ4q0kAE43ACIACzJiSr8AbhtitWC2qxatQV0tZwAhDzlvDlpCri7gvDCImLjEsQCJoJLggDcwUhwoMCYAI1pYiMhhJnFJZpXVy2m3Hf3aOYSkv2Xrku0KmrqGy/fLDqtLqA6xXbqlBzOERDLwKUYZJaTKy3TLZM65fJUBCIsGrT5VWqkepNJEA4HoHhAA
The `is MyClass c` produces a smaller code (which could be better for inlining the function), but I agree it could be a problem if the method is called often with the case where the cast is mostly false and it would be better to have it fail.
Is it possible to run the benchmark on this PR to see if there is any measurable impact?
@NightOwl888 For the readonly vs structs: had that issue before on our code-base as well :)
It also seems like there are plenty of opportunities to reduce allocations by using structs, will check next week when I've some time again if I notice anything on our own usage of Lucene.NET.
I'll take a look on the readme - somehow I missed that when I first glanced through it... As we also use Azure Devops, should be fine to configure it on our account there!
----------------------------------------------------------------
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
[GitHub] [lucenenet] jeme edited a comment on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
jeme edited a comment on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-697372724
@NightOwl888 While it does end up initializing a local variable, in this case it will be a allocation on the stack though, and as far as i understand, this virtually has no cost associated with it when you wan 't to reclaim it.
It's my understanding that when you enter a method, a stack frame is allocated and all you do when you exit the method again is rewind your stack pointer to where it was when you entered. So it's only the allocation it self that has a cost, not the collection of it.
(This is perhaps a bit simplified but I think it brings the understanding across)
----------------------------------------------------------------
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
[GitHub] [lucenenet] NightOwl888 commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-697165228
I tried [running the tests](https://dev.azure.com/shad0962/Experiments/_build/results?buildId=949&view=results), but in the current incarnation, this doesn't build. It is failing to [set the `Debugging.AssertsEnabled` to `true`](https://github.com/apache/lucenenet/blob/de5251c848b79223d394778cbb17ffc1388877ba/src/Lucene.Net.TestFramework/Support/Util/LuceneTestFrameworkInitializer.cs#L228) when running the test framework.
----------------------------------------------------------------
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
[GitHub] [lucenenet] NightOwl888 commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696770379
> change (a != null && a is MyClass) to a is MyClass mc
Careful, although VS2019 tries to get you to do this, I am fairly certain it reduces performance (need some benchmarks to verify). The optimizations in Lucene are verbose, but they are often more performant than the shorter syntax. I saw at least 1 place where the check for null first before the check for the cast had a major impact on performance.
Also, if there are many cases where the cast `a is MyClass mc` will not pass, we are throwing away an extra variable declaration that wouldn't be needed if it were inside of the `if` block. Ideally, we would not allocate memory unless absolutely necessary.
That said, `is null` instead of `== null` doesn't have a huge performance impact and is more robust because it circumvents any `==` operator overload.
> Also changed a few fields to read-only, and one regex variable to static as suggested by the analyzer too.
Usually this is the correct decision. Look out for `readonly` on `struct`, though - I have seen it have a seriously negative impact on performance. Do note that Java doesn't have structs, so we should also be looking for opportunities for them to improve performance.
> Regarding your previous comment, do you think this kind of changes needs the "LUCENENET" comments you mentioned?
In general, if the change is due to a syntax or platform difference between .NET and Java, the comment isn't strictly necessary. If the change actually reflects a change to Lucene's design, then it should definitely be commented.
> By the way, is there any docs or quick start on running the tests locally? And are the tests run automatically on PR, or manually? Just to know if I should bother trying to set it up locally, or easier to let them run and just check the results...
The best way (although it takes longer) is to run it on Azure DevOps. You can set it up on any free Azure DevOps account by following the instructions on the [README](https://github.com/apache/lucenenet/blob/master/README.md#building-and-testing).
Due to permission issues, we cannot enable Azure DevOps to run on PRs, so they should be run first before you submit the PR and we will run them again several times before approving it.
Careful, we don't usually want to make broad sweeping changes like this without thorough testing, otherwise it could take days to figure out why some of the tests don't pass. The tests depend directly on the asserts, so already this could lead to problems if it is not fully verified.
----------------------------------------------------------------
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
[GitHub] [lucenenet] jeme commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
jeme commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-697372724
@NightOwl888 While it does end up initializing a local variable, in this case it will be a allocation on the stack though, and as far as i understand, this virtually has no cost associated with it when you wan 't to reclaim it.
It's my understanding that when you enter a method, a stack frame is allocated and all you do when you exit the method again is rewind your stack pointer to where it was when you entered. So it's only the allocation it self that has a cost, not the collection of it.
----------------------------------------------------------------
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
[GitHub] [lucenenet] theolivenbaum commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [lucenenet] NightOwl888 commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696844345
Interesting. I suspect they wouldn't produce the same IL if the type in the comparison had the `!=` operator overloaded - then it may produce very different results, and in fact depending on the implementation it might not produce expected results. `is` is safer because it doesn't take the operator overloads into account.
That said, I am on board with elad in that using `== object` is not very intuitive for a `null` check. I prefer:
```c#
public static void A(IMyInterface c) {
if(!(c is null) && c is MyClass cc) {
Console.WriteLine(cc.GetType());
}
}
```
----------------------------------------------------------------
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
[GitHub] [lucenenet] NightOwl888 commented on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-697119925
@jeme
Thanks Jens. Looks like a `null` check has a significant impact if many cases are `null`. However, the cases I was worried about look more like:
```c#
object something = cache.Value;
if (something is Value1Type)
{
Value1Type value1 = (Value1Type)something; // <- Allocation happens here (conditionally)
// do something with value1 and return
return value1.Get();
}
else if (something is Value2Type)
{
Value2Type value2 = (Value2Type)something; // <- Allocation happens here (conditionally)
// do something with value2 and return
return value2.Get();
}
return new ARealValue(something);
```
If a large number of cases are `Value2Type` (or even neither `Value1Type` or `Value2Type`) what I am wondering is do we waste memory allocation on `Value1Type` if it uses pattern matching (`if (something is Value1Type value1)`)?
```c#
object something = cache.Value;
if (something is Value1Type value1) // <- Allocation happens here (always)
{
// do something with value1 and return
return value1.Get();
}
else if (something is Value2Type value2) // <- Allocation happens here (always when first if condition fails)
{
// do something with value2 and return
return value2.Get();
}
// If we are here, we are throwing out 2 unnecessary memory allocations
return new ARealValue(something);
```
It seems like we are declaring `Value1Type value1 = default` and then throwing its value out when the `if` condition fails, but perhaps the IL is compensating and converting it back to the above behind the scenes...? I suspect not because the `value1` variable can be used outside of the `if` block even if the condition fails.
This would be especially important in `Equals()` methods where we want to shunt to `false` as quickly as possible with as few allocations as possible.
It looks like you have covered that case in your benchmarks above, but the important metric to consider is the memory allocated rather than the execution time, as memory allocations will cause GC to happen more frequently, which may negate any gain in raw performance. `null` reference type memory allocations [are either 4 or 8 bytes](https://stackoverflow.com/questions/3801878/how-much-memory-does-null-pointer-use) of extra RAM, so they will contribute to the total amount of memory pressure even if they are never assigned a real value.
I have run a benchmark, and even after cranking it up to 1 billion iterations, it seems that either this is not a significant thing to worry about or BenchmarkDotNet does not consider `null` reference type allocations when it measures the results.
``` ini
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18363.1016 (1909/November2018Update/19H2)
Intel Core i7-8850H CPU 2.60GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=3.1.301
[Host] : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
DefaultJob : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
```
| Method | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|-------------------------------------- |---------:|----------:|----------:|------:|------:|------:|----------:|
| PatternMatching_Class1Set_Class2Set | 5.935 ms | 0.1155 ms | 0.1581 ms | - | - | - | - |
| PatternMatching_Class1Null_Class2Set | 6.005 ms | 0.1184 ms | 0.1945 ms | - | - | - | - |
| PatternMatching_Class1Null_Class2Null | 5.903 ms | 0.1120 ms | 0.1333 ms | - | - | - | - |
| NonMatching_Class1Set_Class2Set | 5.915 ms | 0.1148 ms | 0.1410 ms | - | - | - | - |
| NonMatching_Class1Null_Class2Set | 5.874 ms | 0.1168 ms | 0.1637 ms | - | - | - | - |
| NonMatching_Class1Null_Class2Null | 5.897 ms | 0.1133 ms | 0.1391 ms | - | - | - | - |
```c#
public class Class1 { }
public class Class2 { }
[MemoryDiagnoser]
public class AllocationBenchmarks
{
public static readonly Class1 class1Null = null;
public static readonly Class1 class1Set = new Class1();
public static readonly Class2 class2Null = null;
public static readonly Class2 class2Set = new Class2();
public const int Iterations = 1000000000;
[Benchmark]
public void PatternMatching_Class1Set_Class2Set()
{
for (int i = 0; i < Iterations; i++)
PatternMatching(class1Set, class2Set);
}
[Benchmark]
public void PatternMatching_Class1Null_Class2Set()
{
for (int i = 0; i < Iterations; i++)
PatternMatching(class1Null, class2Set);
}
[Benchmark]
public void PatternMatching_Class1Null_Class2Null()
{
for (int i = 0; i < Iterations; i++)
PatternMatching(class1Null, class2Null);
}
[Benchmark]
public void NonMatching_Class1Set_Class2Set()
{
for (int i = 0; i < Iterations; i++)
NonMatching(class1Set, class2Set);
}
[Benchmark]
public void NonMatching_Class1Null_Class2Set()
{
for (int i = 0; i < Iterations; i++)
NonMatching(class1Null, class2Set);
}
[Benchmark]
public void NonMatching_Class1Null_Class2Null()
{
for (int i = 0; i < Iterations; i++)
PatternMatching(class1Null, class2Null);
}
public static void PatternMatching(Class1 class1, Class2 class2)
{
if (class1 is Class1 c1)
{
}
else if (class2 is Class2 c2)
{
}
}
public static void NonMatching(Class1 class1, Class2 class2)
{
if (class1 is Class1)
{
Class1 c1 = (Class1)class1;
}
else if (class2 is Class2)
{
Class2 c2 = (Class2)class2;
}
}
}
```
----------------------------------------------------------------
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
[GitHub] [lucenenet] jeme edited a comment on pull request #345: Reduce casting
Posted by GitBox <gi...@apache.org>.
jeme edited a comment on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696880214
----------------------------------------------------------------
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