Patch w/ test & fix. I also noticed we were not recording a thread
binding when we had an uncontended minThreadState, causing an
unnecessary call to minContentedThreadState for each doc.

I think in general this looks good to me. There is a small chance that the thread affinity will not work corretly if we reset while some other thread is already calling "minThreadState.lock()" we need to put the thread / state binding back into the map if the locked state is not initialized like:

minThreadState.lock();
if (minThreadState.isInitialized() == false) {
  threadBindings.put(requestingThread, minThreadState); // make sure we get the same state next time 
}
return minThreadState;

Ahh, good catch Simon; I'll add that & commit shortly.

Commit 1592620 from Michael McCandless in branch 'dev/trunk'
[ https://svn.apache.org/r1592620 ]

LUCENE-5644: clear IW's thread state bindings on flush

Commit 1592621 from Michael McCandless in branch 'dev/branches/branch_4x'
[ https://svn.apache.org/r1592621 ]

LUCENE-5644: clear IW's thread state bindings on flush

Commit 1592623 from Michael McCandless in branch 'dev/branches/lucene_solr_4_8'
[ https://svn.apache.org/r1592623 ]

LUCENE-5644: clear IW's thread state bindings on flush

I'm reopening this, something is wrong with the fix ... when I index Geonames with 2 threads I sometimes see strange sized segments on flush, e.g.:

DWPT 0 [Tue May 06 10:45:11 EDT 2014; main]: flush postings as segment _26 numDocs=1
DWPT 0 [Tue May 06 10:45:11 EDT 2014; Thread-0]: flush postings as segment _25 numDocs=15872

After struggling for some time trying to fix the thread hazards
between flush & getAndLock, I decided to simply remove
ThreadAffinityDocumentsWriterThreadPool.

I think it's too hairy/complex. The original motivation for this was
for when apps use a different thread to index different content
sources; this way we send similar docs to the same thread state and
should get better RAM efficiency.

But, 1) I think it's uncommon for apps to do this (they typically use
a "blind" thread pool) so it's fragile, 2) it's not clear really how
much perf gain this is (likely minor), and 3) it's too hard to get this working
correctly (this issue).

So instead I made DocumentsWriterPerThreadPool final, and I impl'd a
simple LIFO queue, and fixed the two places (in DW) that obtain a
thread state for indexing to use a new DWPTP.release API that puts the
state back into the free list & notifies for any waiting threads to
wake up.

Note that other places that lock a thread state (e.g. flush/abort)
don't use this free list; I think this is OK?

Lucene tests pass, but this is a big change, and we need to scrutinize
/ test it more.

It seems to behave properly when I index Geonames w/ N threads, i.e. I
get N segments flushed by getReader, but I'll test more ... I'll also
make a standalone test case that validates this.

This is just an initial patch quickly coded up ... javadocs are wrong, etc.

I suspect on abort I need to wake up any indexing threads waiting on the pool...

But, 1) I think it's uncommon for apps to do this (they typically use
a "blind" thread pool) so it's fragile

I don't know how uncommon it is ... I'm thinking crawlers, where each thread crawls a separate data source. Of course there are crawlers that operate on a pool of resources too. So basically if an app has a thread-affinity logic, with this patch it cannot take advantage of it at all.

Another usecase where I thought about thread-affinity is if you want to e.g. create an index in a special manner, for instance a pre-sorted index. So say that you aggregate a bunch of documents, then pre-sort them and add them one at a time, with the intention to have a pre-sorted segment. With this patch it's impossible right?

I agree that we need something simple in Lucene, and if thread-affinity is hard to write/understand/maintain, I'm all for removing it in favor of this simpler model. But I also think this should be pluggable, even if it's uber-expert, package-private plugin. So maybe we can factor out a simple interface/abstract class DWThreadPool and have a concrete final LIFOThreadPool implementation? This will allow apps to specialize if they want?

I reviewed the patch and I think it's good. But I want to scrutinize it more.

Commit 1593023 from Michael McCandless in branch 'dev/trunk'
[ https://svn.apache.org/r1593023 ]

LUCENE-5644: add test case

Commit 1593024 from Michael McCandless in branch 'dev/branches/branch_4x'
[ https://svn.apache.org/r1593024 ]

LUCENE-5644: add test case

Commit 1593026 from Michael McCandless in branch 'dev/branches/lucene_solr_4_8'
[ https://svn.apache.org/r1593026 ]

LUCENE-5644: add test case

Commit 1593027 from Michael McCandless in branch 'dev/trunk'
[ https://svn.apache.org/r1593027 ]

LUCENE-5644: i moved this test over to new class

Commit 1593028 from Michael McCandless in branch 'dev/branches/branch_4x'
[ https://svn.apache.org/r1593028 ]

LUCENE-5644: i moved this test over to new class

Commit 1593029 from Michael McCandless in branch 'dev/branches/lucene_solr_4_8'
[ https://svn.apache.org/r1593029 ]

LUCENE-5644: i moved this test over to new class

Commit 1593037 from Michael McCandless in branch 'dev/trunk'
[ https://svn.apache.org/r1593037 ]

LUCENE-5644: add test case

Commit 1593038 from Michael McCandless in branch 'dev/branches/branch_4x'
[ https://svn.apache.org/r1593038 ]

LUCENE-5644: add test case

Commit 1593039 from Michael McCandless in branch 'dev/branches/lucene_solr_4_8'
[ https://svn.apache.org/r1593039 ]

LUCENE-5644: add test case

Another usecase where I thought about thread-affinity is if you want to e.g. create an index in a special manner, for instance a pre-sorted index. So say that you aggregate a bunch of documents, then pre-sort them and add them one at a time, with the intention to have a pre-sorted segment. With this patch it's impossible right?

For that to work right you need to use only 1 thread right? I don't
think this patch will break it?

But I also think this should be pluggable

How about I make this default impl un-final?

New patch, folding in Shai's feedback, removing the nocommit, and adding a notifyAll() to wake up any indexing threads (but my test case failed to uncover deadlock here...).

I'm running distributed beasting of all Lucene's core + modules tests ....

I beasted Lucene's core+module tests 48 times and didn't hit any fails except https://issues.apache.org/jira/browse/LUCENE-5652 (unrelated)...

I like the patch much simpler though... a couple of comments

• can we remove the sync keyword from newThreadState now? it seems unnecessary?
• can we call notifyall across the board seems safer?
• in the look instead of break can we lock and return the state it seems like it's more clear that way since we only lock if we get it form the free list?
• I'd love in general to not sync on "this" but rather have a lock obj or a condition but that's just taste I guess...
• in release can we unlock outside the sync block?

in general that looks good though. I wonder if we really should make it final but that's a different story...

Thanks Simon, I'm folding these in ...

in the look instead of break can we lock and return the state it seems like it's more clear that way since we only lock if we get it form the free list?

The challenge is I want to .lock() outside of the sync block ...

For that to work right you need to use only 1 thread right? I don't
think this patch will break it?

Not necessarily. You can have different threads batch up different documents and pre-sort them before .addDocument. Then you end up w/ pre-sorted segments (if you have Thread-affinity). I'm not saying it's such an important usecase to support out-of-the-box, more that there is such a usecase, and I think it'd be better if we offer some way in Lucene to enable it.

How about I make this default impl un-final?

Either that or factor out a simple API, your call. I haven't looked at the API closely yet, but my feeling is that if we keep the previous abstract class and a final LIFOPool class, that would do it? When the base class is not abstract and final, I worry that at some point we'll worry about back-compat issues etc. A final class makes it easier for us to assume things. But, I leave it to you.

Not necessarily. You can have different threads batch up different documents and pre-sort them before .addDocument. Then you end up w/ pre-sorted segments (if you have Thread-affinity). I'm not saying it's such an important usecase to support out-of-the-box, more that there is such a usecase, and I think it'd be better if we offer some way in Lucene to enable it.

OK, I see, you're right. But this (which segment your thread sends
its docs to) was never really a hard guarantee with IW; it's really an
impl detail.

How about I make this default impl un-final?

Either that or factor out a simple API, your call.

I'd like to defer this: I'll open a separate issue. I agree it's
dangerous to have the one impl not be final, so I put final back for
now, and in the follow on issue we can make it un-final and abstract
again, and maybe we can get a (correct) thread-affinity impl back. I
just think tweaking with the thread assignment is very very expert.

I'd love in general to not sync on "this" but rather have a lock obj or a condition but that's just taste I guess...

I'd rather not add another lock here (this class is already using its
intrinsic lock)? Or at least, let's defer it; maybe we can make a
lock-free queue later...

New patch, folding in all feedback. I think it's ready; I'll commit soon.

But ant precommit is currently failing for other reasons:

BUILD FAILED
/l/fifo/build.xml:92: The following files contain @author tags, tabs or nocommits:
* solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java

Commit 1593226 from Michael McCandless in branch 'dev/trunk'
[ https://svn.apache.org/r1593226 ]

LUCENE-5644: switch to simpler LIFO thread to ThreadState allocator during indexing

Commit 1593228 from Michael McCandless in branch 'dev/branches/branch_4x'
[ https://svn.apache.org/r1593228 ]

LUCENE-5644: switch to simpler LIFO thread to ThreadState allocator during indexing

Commit 1593230 from Michael McCandless in branch 'dev/branches/lucene_solr_4_8'
[ https://svn.apache.org/r1593230 ]

LUCENE-5644: switch to simpler LIFO thread to ThreadState allocator during indexing

Commit 1593240 from Michael McCandless in branch 'dev/branches/lucene_solr_4_8'
[ https://svn.apache.org/r1593240 ]

LUCENE-5644: remove this class

Reopening ...

I realized the simple LIFO logic is too simple, right after a flush. When that happens, we should try to pick a ThreadState that's already initialized, so that if no full-flushing (getReader) is happening, we don't tie up RAM indefinitely in the pending segments.

Patch w/ test; the core change is quite minor, in the case where the ThreadState isn't initialized, we loop through the other thread states to find one that is initialized.

Commit 1593649 from Michael McCandless in branch 'dev/branches/lucene_solr_4_8'
[ https://svn.apache.org/r1593649 ]

LUCENE-5644: favor an already initialized ThreadState

Commit 1593650 from Michael McCandless in branch 'dev/branches/branch_4x'
[ https://svn.apache.org/r1593650 ]

LUCENE-5644: favor an already initialized ThreadState

Commit 1593651 from Michael McCandless in branch 'dev/trunk'
[ https://svn.apache.org/r1593651 ]

LUCENE-5644: favor an already initialized ThreadState