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/11/04 03:41:52 UTC
[GitHub] [lucenenet] NightOwl888 commented on pull request #371: Add new interface to NumericDocValues to close #370
NightOwl888 commented on pull request #371:
URL: https://github.com/apache/lucenenet/pull/371#issuecomment-721494674
Thanks. I finally had a chance to take a close look at this. Since adding the delegate was a recent change in https://github.com/apache/lucenenet/pull/348/commits/34758c1315391794825f33a3f48b5a3dd6083745 (pull #348), I ran benchmarks both on reverting that commit vs the changes in this PR (both rebased against master).
## Benchmarks
### IndexFiles
<details>
<summary>Click to expand</summary>
``` ini
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18363.1082 (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.403
[Host] : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00005 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00006 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00007 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00008 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00009 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00010 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00011 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00012 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-wip00_before_GH-370 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-wip01_revertdelegates_GH-370 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-wip02_refactorfieldcache_GH-370 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
InvocationCount=1 IterationCount=15 LaunchCount=2
UnrollFactor=1 WarmupCount=10
```
| Method | Job | NuGetReferences | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|----------- |-------------------------------------- |---------------------------------------------- |---------:|---------:|---------:|-----------:|----------:|----------:|----------:|
| IndexFiles | 4.8.0-beta00005 | Lucene.Net.Analysis.Common 4.8.0-beta00005 | 687.7 ms | 12.01 ms | 16.44 ms | 43000.0000 | 8000.0000 | 7000.0000 | 220.89 MB |
| IndexFiles | 4.8.0-beta00006 | Lucene.Net.Analysis.Common 4.8.0-beta00006 | 683.8 ms | 15.25 ms | 21.88 ms | 43000.0000 | 8000.0000 | 7000.0000 | 220.94 MB |
| IndexFiles | 4.8.0-beta00007 | Lucene.Net.Analysis.Common 4.8.0-beta00007 | 680.3 ms | 13.02 ms | 19.49 ms | 44000.0000 | 8000.0000 | 7000.0000 | 221 MB |
| IndexFiles | 4.8.0-beta00008 | Lucene.Net.Analysis.Common 4.8.0-beta00008 | 672.6 ms | 13.61 ms | 18.64 ms | 44000.0000 | 8000.0000 | 7000.0000 | 221.39 MB |
| IndexFiles | 4.8.0-beta00009 | Lucene.Net.Analysis.Common 4.8.0-beta00009 | 704.5 ms | 35.29 ms | 51.73 ms | 44000.0000 | 8000.0000 | 7000.0000 | 221.28 MB |
| IndexFiles | 4.8.0-beta00010 | Lucene.Net.Analysis.Common 4.8.0-beta00010 | 674.4 ms | 11.88 ms | 17.41 ms | 44000.0000 | 8000.0000 | 7000.0000 | 221.23 MB |
| IndexFiles | 4.8.0-beta00011 | Lucene.Net.Analysis.Common 4.8.0-beta00011 | 675.7 ms | 13.38 ms | 19.61 ms | 44000.0000 | 8000.0000 | 7000.0000 | 221.29 MB |
| IndexFiles | 4.8.0-beta00012 | Lucene.Net.Analysis.Common 4.8.0-beta00012 | 701.7 ms | 9.53 ms | 13.05 ms | 56000.0000 | 7000.0000 | 6000.0000 | 287 MB |
| IndexFiles | 4.8.0-wip00_before_GH-370 | Lucene.Net.Analysis.Common 4.8.0-ci0000002208 | 673.6 ms | 7.85 ms | 11.00 ms | 43000.0000 | 8000.0000 | 7000.0000 | 220.03 MB |
| IndexFiles | 4.8.0-wip01_revertdelegates_GH-370 | Lucene.Net.Analysis.Common 4.8.0-ci0000002212 | 673.7 ms | 20.74 ms | 29.08 ms | 44000.0000 | 8000.0000 | 7000.0000 | 220.16 MB |
| IndexFiles | 4.8.0-wip02_refactorfieldcache_GH-370 | Lucene.Net.Analysis.Common 4.8.0-ci0000002213 | 671.3 ms | 15.92 ms | 22.31 ms | 44000.0000 | 8000.0000 | 7000.0000 | 220.17 MB |
</details>
### SearchFiles
<details>
<summary>Click to expand</summary>
``` ini
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18363.1082 (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.403
[Host] : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00005 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00006 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00007 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00008 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00009 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00010 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00011 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-beta00012 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-wip00_before_GH-370 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-wip01_revertdelegates_GH-370 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
4.8.0-wip02_refactorfieldcache_GH-370 : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
IterationCount=15 LaunchCount=2 WarmupCount=10
```
| Method | Job | NuGetReferences | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------ |-------------------------------------- |---------------------------------------------------------------------------------------- |----------:|---------:|----------:|----------:|---------:|------:|----------:|
| SearchFiles | 4.8.0-beta00005 | Lucene.Net.Analysis.Common 4.8.0-beta00005,Lucene.Net.QueryParser 4.8.0-beta00005 | 142.74 ms | 3.010 ms | 4.317 ms | 8750.0000 | 500.0000 | - | 41.39 MB |
| SearchFiles | 4.8.0-beta00006 | Lucene.Net.Analysis.Common 4.8.0-beta00006,Lucene.Net.QueryParser 4.8.0-beta00006 | 145.19 ms | 4.234 ms | 6.206 ms | 8750.0000 | 500.0000 | - | 41.37 MB |
| SearchFiles | 4.8.0-beta00007 | Lucene.Net.Analysis.Common 4.8.0-beta00007,Lucene.Net.QueryParser 4.8.0-beta00007 | 145.13 ms | 3.663 ms | 5.253 ms | 8750.0000 | 250.0000 | - | 41.26 MB |
| SearchFiles | 4.8.0-beta00008 | Lucene.Net.Analysis.Common 4.8.0-beta00008,Lucene.Net.QueryParser 4.8.0-beta00008 | 93.55 ms | 9.737 ms | 14.273 ms | 8666.6667 | 500.0000 | - | 40.33 MB |
| SearchFiles | 4.8.0-beta00009 | Lucene.Net.Analysis.Common 4.8.0-beta00009,Lucene.Net.QueryParser 4.8.0-beta00009 | 86.96 ms | 2.683 ms | 3.848 ms | 8666.6667 | 500.0000 | - | 40.33 MB |
| SearchFiles | 4.8.0-beta00010 | Lucene.Net.Analysis.Common 4.8.0-beta00010,Lucene.Net.QueryParser 4.8.0-beta00010 | 93.86 ms | 2.843 ms | 4.256 ms | 8600.0000 | - | - | 40.2 MB |
| SearchFiles | 4.8.0-beta00011 | Lucene.Net.Analysis.Common 4.8.0-beta00011,Lucene.Net.QueryParser 4.8.0-beta00011 | 93.08 ms | 3.083 ms | 4.518 ms | 8666.6667 | 500.0000 | - | 40.19 MB |
| SearchFiles | 4.8.0-beta00012 | Lucene.Net.Analysis.Common 4.8.0-beta00012,Lucene.Net.QueryParser 4.8.0-beta00012 | 97.00 ms | 5.825 ms | 8.719 ms | 8833.3333 | 333.3333 | - | 40.81 MB |
| SearchFiles | 4.8.0-wip00_before_GH-370 | Lucene.Net.Analysis.Common 4.8.0-ci0000002208,Lucene.Net.QueryParser 4.8.0-ci0000002208 | 86.74 ms | 0.621 ms | 0.891 ms | 7000.0000 | 333.3333 | - | 33.17 MB |
| SearchFiles | 4.8.0-wip01_revertdelegates_GH-370 | Lucene.Net.Analysis.Common 4.8.0-ci0000002212,Lucene.Net.QueryParser 4.8.0-ci0000002212 | 85.16 ms | 1.542 ms | 2.307 ms | 7000.0000 | 333.3333 | - | 33.17 MB |
| SearchFiles | 4.8.0-wip02_refactorfieldcache_GH-370 | Lucene.Net.Analysis.Common 4.8.0-ci0000002213,Lucene.Net.QueryParser 4.8.0-ci0000002213 | 84.36 ms | 1.144 ms | 1.677 ms | 7000.0000 | 333.3333 | - | 33.17 MB |
</details>
While the results do show a minor performance gain, the margin of error accounts for nearly all of it. Unlike removing other delegates in #383, we are not seeing a significant drop in memory allocations when doing "basic" benchmarks.
## Failing Tests
This PR has introduced 5 new failing tests.
You can view the details of these failures at https://dev.azure.com/LuceneNET-Temp/Lucene.NET/_build/results?buildId=1170&view=ms.vss-test-web.build-test-results-tab
## Double-caching
While I don't know for a fact that this is a problem, the comments indicate these values are already cached.
```c#
// Not cached here by FieldCacheImpl (cached instead
// per-thread by SegmentReader):
```
Digging into `SegmentReader`, there are 2 caches both of which use `ThreadLocal<T>`. I suspect that one of those caches is what the comment refers to. Putting these instances into a static variable and reusing them across threads seems like it might introduce a thread safety problem.
## Summary
Basically, our three options boil down to:
1. Leave `FieldCacheImpl` as-is
2. Revert https://github.com/apache/lucenenet/pull/348/commits/34758c1315391794825f33a3f48b5a3dd6083745
3. Fix the failing tests and keep this design change
Changing to use delegates rather than using concrete subclasses was done deliberately to reduce the number of classes to maintain. So, the choice with reverting https://github.com/apache/lucenenet/pull/348/commits/34758c1315391794825f33a3f48b5a3dd6083745 is raw performance vs maintainability. I am on the fence, but since the gains of reverting it are marginal at best, I am leaning toward leaving it as-is.
Changing the design to this PR means diverging from the Lucene source. It will be slightly more of a maintenance burden to upgrade to the next version of Lucene. While this is something that can be seriously considered if the performance gains are significant enough, there doesn't seem to be enough of a gain here to warrant the design change (not to mention the failing tests). These benchmarks are very narrowly scoped, though. If there is another metric that can be benchmarked that shows a significant gain, then we can reconsider.
That being said, don't be discouraged by this. While we haven't been able to use very many of your commits so far, the fact that you are identifying new issues that were previously unknown makes your contributions to Lucene.NET valuable. We have already improved search performance by around 10% over 4.8.0-beta00012 thanks in part to your help.
----------------------------------------------------------------
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