RavenDB-22088 - reduce the lock contention in SegmentReader #67
Conversation
| lock (this) | ||
| { | ||
| // This is initialized in the constructor. | ||
| // The only time it isn't initialized is when termsIndexDivisor equals -1, which we don't use. |
There was a problem hiding this comment.
What we don't use? Can you elaborate on that?
There was a problem hiding this comment.
The TermInfosReader holds an array of sorted, cached terms and their positions (note that this cache does not include all terms) to efficiently locate the correct term during searches.
The algorithm is roughly the following:
We use binary search to find the position of the closest term within the sorted cache.
We then iterate until we find the exact match.
The size of the cache is determined by the termsIndexDivisor. A larger number creates a smaller cache, requiring more time to find the correct term but reducing memory usage.
The default value is 1.
There was a problem hiding this comment.
It's hard for me understand the answer. I was rather expecting you to tell my why is it the case that in our scenarios termsIndexDivisor is never -1
There was a problem hiding this comment.
I can confirm that it can never be -1:
lucenenet/src/Lucene.Net/Index/IndexWriter.cs
Line 1522 in 34bc333
| { | ||
| // This is initialized in the constructor. | ||
| // The only time it isn't initialized is when termsIndexDivisor equals -1, which we don't use. | ||
| var reader = tis; |
There was a problem hiding this comment.
What if tis will be already disposed? As I see it can be disposed by DecRef() and since here we're no longer under the lock that seems to be possible
There was a problem hiding this comment.
We can still get a reference to it and after that it will be disposed.
There is no protection against it in the current implementation.
There was a problem hiding this comment.
In current impl if tis is null (which is the case after it got disposed:
lucenenet/src/Lucene.Net/Index/SegmentReader.cs
Lines 261 to 266 in 34bc333
then we return tisNoIndex.
After the change made here we can return tis while DecRef() will dispose it (don't know if those can be executed concurrently but I believe they can since both use lock())
There was a problem hiding this comment.
You can get a reference to it (when calling GetTermsReader).
And after that it can be disposed, so you'll be holding a disposed object.
There was a problem hiding this comment.
The original code doesn't protect you from getting a tis this might be disposed afterwards.
| return tis != null; | ||
| } | ||
| { | ||
| // If this is null, we call LoadTermsIndex, which uses a lock. |
There was a problem hiding this comment.
Please note that LoadTermsIndex has a comment "This method is not fully thread safe, and relies on the synchronization in IndexWriter". I wonder if we can relay on LoadTermsIndex and the lock we have there, especially after the change you made there.
There was a problem hiding this comment.
The lock is there because this method isn't thread safe.
There was a problem hiding this comment.
And your change made it even more not thread save, didn't it?
There was a problem hiding this comment.
The lock was added there along with the comment.
Maybe I'm missing something but this the lock makes this method thread safe.
| @@ -310,7 +317,10 @@ internal void DecRef() | |||
| } | |||
|
|
|||
| internal void OpenDocStores(SegmentInfo si, IState state) | |||
There was a problem hiding this comment.
Was this method also affected (and you saw that in the stacktraces) or you modified it "just in case" because it might have the same issue?
There was a problem hiding this comment.
I added Console.WriteLine in all methods in order to see which ones are being used.
I don't see a reason to hold the lock if fieldsReaderOrig != null.
|
We talked with Grisha about all my comments. It looks fine although it's a risky change IMO. We'll see how it'll work on RavenDB's tests. |
No description provided.