Reduce contention in DocumentsWriterPerThreadPool. #12199
Conversation
Obtaining a DWPT and putting it back into the pool is subject to contention. This change reduces contention by using 8 sub pools that are tried sequentially. When applied on top of apache#12198, this reduces the time to index geonames with 20 threads from ~19s to ~16-17s.
| dwpt = newWriter(); | ||
| ensureOpen(); | ||
| DocumentsWriterPerThread dwpt = freeList.poll(DocumentsWriterPerThread::tryLock); | ||
| if (dwpt == null) { |
There was a problem hiding this comment.
For some reason, double locking always makes me cringe. There are lengthy discussions about this idiom all around the web (visibility of partially constructed objects).
There was a problem hiding this comment.
I don't think that this is a case of double-checked locking. In general double-checked locking tries to reduce the overhead of acquiring a lock by adding a quick check before the lock. Here the logic is different, it would be legal to remove the call to poll under lock, I only added it because there is a chance that a thread had to wait on the lock, so we could check if another DWPT was added to the queue in the meantime in order to save creating a new DWPT. I don't think it's actually important, I could remove the second call to poll to make it look less like double-checked locking.
There was a problem hiding this comment.
It's not about the poll. It's about whether dwpt = newWriter(); can be expanded and reordered by the compiler so that dwpt gets assigned a value before the constructor (or whatever initialization inside newWriter) takes place. Any thread checking dwpt == null outside synchronized could, at least in theory, see such a "partially constructed" object.
There was a problem hiding this comment.
Alex Shipilev had a nice writeup about it - found it here: https://shipilev.net/blog/2014/safe-public-construction/
There was a problem hiding this comment.
Thanks for sharing this blog post, I remember reading it in the past, it was a good re-read. I mentioned polls because I thought that they were what made you think that this code is a case of double-checked locking, as there is a first call before the lock and another one under the lock, like the null checks with double-checked locking with singletons. I need to go on weekend, I'll try to post a convincing explanation of why this is not a case of double-checked locking and why it is safe next week. By the way, thanks for looking!
There was a problem hiding this comment.
No need to convince me, @jpountz - I just expressed my reluctance at it because, well, it requires convincing. :) Unless there's really a huge gain, I typically just go with what I understand works (making the field volatile, for example).
There was a problem hiding this comment.
I still have the feeling that you are making incorrect assumptions about what this piece of code is doing (this method itself doesn't publish the object to other threads), but I took this thread as a call to keep things simple, so I removed the retry and added a few more comments about the logic of this pool.
There was a problem hiding this comment.
I certainly am! But anyone looking at this code without jumping deep will have the same doubts, I think. Unless there is a convincing (performance) argument that it's worth it, I like the updated version a lot better - it's clear and simple.
There was a problem hiding this comment.
Thanks code looks much better. I was also irritated by the long chain of "tries" with and without synchronized. To me actualy code is easier to read.
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java
Outdated
Show resolved
Hide resolved
|
Thanks for the updates. Sorry for nitpicking, I just prefer code that goes sequentially and uses the pattern "exit method as soon as condition mets". This makes it easier to understand. Nice that there's no double locking anymore. |
|
|
||
| // Only used for assertions | ||
| boolean contains(Object o) { | ||
| for (int i = 0; i < CONCURRENCY; ++i) { |
There was a problem hiding this comment.
nitpick can you add a check that assertions are enabled?
| // Only used for assertions | ||
| boolean contains(Object o) { | ||
| for (int i = 0; i < CONCURRENCY; ++i) { | ||
| locks[i].lock(); |
There was a problem hiding this comment.
this is really a nit pick but for methods that use a lock I'd prefer to assign the lock to a local var instead of dereferencing it again in the finally block.
|
|
||
| boolean remove(Object o) { | ||
| for (int i = 0; i < CONCURRENCY; ++i) { | ||
| locks[i].lock(); |
There was a problem hiding this comment.
same here, maybe use a local var for the lock. It really looks cleaner
Obtaining a DWPT and putting it back into the pool is subject to contention. This change reduces contention by using 8 sub pools that are tried sequentially. When applied on top of #12198, this reduces the time to index geonames with 20 threads from ~19s to ~16-17s.
|
I suspect this change to be the source of the speedup when indexing vectors on https://home.apache.org/~mikemccand/lucenebench/indexing.html, but maybe more because of the introduced affinity between indexing threads and DWPTs than because of reduced contention since contention generally shows up when indexing is fast, which isn't the case with vectors? |
After upgrading Elasticsearch to a recent Lucene snapshot, we observed a few indexing slowdowns when indexing with low numbers of cores. This appears to be due to the fact that we lost too much of the bias towards larger DWPTs in apache#12199. This change tries to add back more ordering by adjusting the concurrency of `DWPTPool` to the number of cores that are available on the local node.
After upgrading Elasticsearch to a recent Lucene snapshot, we observed a few indexing slowdowns when indexing with low numbers of cores. This appears to be due to the fact that we lost too much of the bias towards larger DWPTs in apache#12199. This change tries to add back more ordering by adjusting the concurrency of `DWPTPool` to the number of cores that are available on the local node.
After upgrading Elasticsearch to a recent Lucene snapshot, we observed a few indexing slowdowns when indexing with low numbers of cores. This appears to be due to the fact that we lost too much of the bias towards larger DWPTs in #12199. This change tries to add back more ordering by adjusting the concurrency of `DWPTPool` to the number of cores that are available on the local node.
After upgrading Elasticsearch to a recent Lucene snapshot, we observed a few indexing slowdowns when indexing with low numbers of cores. This appears to be due to the fact that we lost too much of the bias towards larger DWPTs in #12199. This change tries to add back more ordering by adjusting the concurrency of `DWPTPool` to the number of cores that are available on the local node.
After upgrading Elasticsearch to a recent Lucene snapshot, we observed a few indexing slowdowns when indexing with low numbers of cores. This appears to be due to the fact that we lost too much of the bias towards larger DWPTs in apache#12199. This change tries to add back more ordering by adjusting the concurrency of `DWPTPool` to the number of cores that are available on the local node.
@jpountz Will this change makes more small segments generated than the synchronized version? |
Obtaining a DWPT and putting it back into the pool is subject to contention. This change reduces contention by using 8 sub pools that are tried sequentially. When applied on top of #12198, this reduces the time to index geonames with 20 threads from ~19s to ~16-17s.