Add new interface to NumericDocValues to close #370 #371
Conversation
|
Thanks. I finally had a chance to take a close look at this. Since adding the delegate was a recent change in 34758c1 (pull #348), I ran benchmarks both on reverting that commit vs the changes in this PR (both rebased against master). BenchmarksIndexFilesClick to expandBenchmarkDotNet=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
SearchFilesClick to expandBenchmarkDotNet=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
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 TestsThis 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-cachingWhile I don't know for a fact that this is a problem, the comments indicate these values are already cached. // Not cached here by FieldCacheImpl (cached instead
// per-thread by SegmentReader):Digging into SummaryBasically, our three options boil down to:
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 34758c1 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. |
|
Hi @NightOwl888 - sorry for the long time since my last answer - but I was off last week on long due holidays :) I think this change is significant for our usage of Lucene, but probably is not captured by the benchmark as they only enumerate some results. On our use-case, we often need to enumerate all hits up to a limit (usually hundreds of thousands) - and read the value of an identifier field to match with our data for further processing - which is what causes the large number of calls to this method. I can try to push a new benchmark to reflect that, but measuring locally running our code directly referencing the Lucene projects instead of the nuget package gave us a significant reduction in allocations. |
|
BTW - no worries reg. the comments - happy to keep contributing on my spare time as possible - will do some catch up on the feedback on the other PRs to see what I missed since I was off! |
|
I just submitted PR #389 and noticed while I was writing it up that you replied here. I am working on getting a release vote started in the next few days, so I put together a PR that reverts the commit 34758c1. Rolling back the change for the release is the safest route, because it has already been thoroughly tested.
Good to hear. That boxing issue was the main culprit - it reduced RAM consumption by around 15%. But changing to use generic parameters on |
|
Fixed in #389 |
As mentioned in #370, we measured a significant number of allocations when reading values from the cache, due to a variable capture inside a lambda and a new FieldCache.T instance being created.
Adding an interface to the class NumericDocValues for each of the supported types instead of using the lambda, and adding a cached FieldCache.T for each type on the NumericDocValues class that is created only on first use.