diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index 33e32615597..b27fce27059 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -653,6 +653,14 @@ Changes in backwards compatibility policy * LUCENE-3541: Remove IndexInput's protected copyBuf. If you want to keep a buffer in your IndexInput, do this yourself in your implementation, and be sure to do the right thing on clone()! (Robert Muir) + +* LUCENE-2822: TimeLimitingCollector now expects a counter clock instead of + relying on a private daemon thread. The global time limiting clock thread + has been exposed and is now lazily loaded and fully optional. + TimeLimitingCollector now supports setting clock baseline manually to include + prelude of a search. Previous versions set the baseline on construction time, + now baseline is set once the first IndexReader is passed to the collector + unless set before. (Simon Willnauer) Changes in runtime behavior diff --git a/lucene/src/java/org/apache/lucene/search/TimeLimitingCollector.java b/lucene/src/java/org/apache/lucene/search/TimeLimitingCollector.java index 63ad23d9d6c..404e6a7bac1 100644 --- a/lucene/src/java/org/apache/lucene/search/TimeLimitingCollector.java +++ b/lucene/src/java/org/apache/lucene/search/TimeLimitingCollector.java @@ -20,6 +20,7 @@ package org.apache.lucene.search; import java.io.IOException; import org.apache.lucene.index.IndexReader.AtomicReaderContext; +import org.apache.lucene.util.Counter; import org.apache.lucene.util.ThreadInterruptedException; /** @@ -30,69 +31,9 @@ import org.apache.lucene.util.ThreadInterruptedException; */ public class TimeLimitingCollector extends Collector { - /** - * Default timer resolution. - * @see #setResolution(long) - */ - public static final int DEFAULT_RESOLUTION = 20; - - /** - * Default for {@link #isGreedy()}. - * @see #isGreedy() - */ - public boolean DEFAULT_GREEDY = false; - - private static long resolution = DEFAULT_RESOLUTION; - - private boolean greedy = DEFAULT_GREEDY ; - - private static final class TimerThread extends Thread { - - // NOTE: we can avoid explicit synchronization here for several reasons: - // * updates to volatile long variables are atomic - // * only single thread modifies this value - // * use of volatile keyword ensures that it does not reside in - // a register, but in main memory (so that changes are visible to - // other threads). - // * visibility of changes does not need to be instantaneous, we can - // afford losing a tick or two. - // - // See section 17 of the Java Language Specification for details. - private volatile long time = 0; - - /** - * TimerThread provides a pseudo-clock service to all searching - * threads, so that they can count elapsed time with less overhead - * than repeatedly calling System.currentTimeMillis. A single - * thread should be created to be used for all searches. - */ - private TimerThread() { - super("TimeLimitedCollector timer thread"); - this.setDaemon( true ); - } - - @Override - public void run() { - while (true) { - // TODO: Use System.nanoTime() when Lucene moves to Java SE 5. - time += resolution; - try { - Thread.sleep( resolution ); - } catch (InterruptedException ie) { - throw new ThreadInterruptedException(ie); - } - } - } - - /** - * Get the timer value in milliseconds. - */ - public long getMilliseconds() { - return time; - } - } /** Thrown when elapsed search time exceeds allowed search time. */ + @SuppressWarnings("serial") public static class TimeExceededException extends RuntimeException { private long timeAllowed; private long timeElapsed; @@ -117,19 +58,12 @@ public class TimeLimitingCollector extends Collector { } } - // Declare and initialize a single static timer thread to be used by - // all TimeLimitedCollector instances. The JVM assures that - // this only happens once. - private final static TimerThread TIMER_THREAD = new TimerThread(); - - static { - TIMER_THREAD.start(); - } - - private final long t0; - private final long timeout; + private long t0 = Long.MIN_VALUE; + private long timeout = Long.MIN_VALUE; private final Collector collector; - + private final Counter clock; + private final long ticksAllowed; + private boolean greedy = false; private int docBase; /** @@ -137,38 +71,45 @@ public class TimeLimitingCollector extends Collector { * @param collector the wrapped {@link Collector} * @param timeAllowed max time allowed for collecting hits after which {@link TimeExceededException} is thrown */ - public TimeLimitingCollector(final Collector collector, final long timeAllowed ) { + public TimeLimitingCollector(final Collector collector, Counter clock, final long ticksAllowed ) { this.collector = collector; - t0 = TIMER_THREAD.getMilliseconds(); - this.timeout = t0 + timeAllowed; + this.clock = clock; + this.ticksAllowed = ticksAllowed; } - - /** - * Return the timer resolution. - * @see #setResolution(long) - */ - public static long getResolution() { - return resolution; - } - + /** - * Set the timer resolution. - * The default timer resolution is 20 milliseconds. - * This means that a search required to take no longer than - * 800 milliseconds may be stopped after 780 to 820 milliseconds. - *
Note that: - * + * Sets the baseline for this collector. By default the collectors baseline is + * initialized once the first reader is passed to + * {@link #setNextReader(AtomicReaderContext)}. To include operations executed + * in prior to the actual document collection set the baseline through this method + * in your prelude. + *

+ * Example usage: + * 

+   *   Counter clock = ...;
+   *   long baseline = clock.get();
+   *   // ... prepare search
+   *   TimeLimitingCollector collector = new TimeLimitingCollector(c, clock, numTicks);
+   *   collector.setBaseline(baseline);
+   *   indexSearcher.search(query, collector);
+   *
+ *

+ * @see #setBaseline() + * @param clockTime */ - public static void setResolution(long newResolution) { - resolution = Math.max(newResolution,5); // 5 milliseconds is about the minimum reasonable time for a Object.wait(long) call. + public void setBaseline(long clockTime) { + t0 = clockTime; + timeout = t0 + ticksAllowed; } - + + /** + * Syntactic sugar for {@link #setBaseline(long)} using {@link Counter#get()} + * on the clock passed to the construcutor. + */ + public void setBaseline() { + setBaseline(clock.get()); + } + /** * Checks if this time limited collector is greedy in collecting the last hit. * A non greedy collector, upon a timeout, would throw a {@link TimeExceededException} @@ -199,7 +140,7 @@ public class TimeLimitingCollector extends Collector { */ @Override public void collect(final int doc) throws IOException { - long time = TIMER_THREAD.getMilliseconds(); + final long time = clock.get(); if (timeout < time) { if (greedy) { //System.out.println(this+" greedy: before failing, collecting doc: "+(docBase + doc)+" "+(time-t0)); @@ -216,6 +157,9 @@ public class TimeLimitingCollector extends Collector { public void setNextReader(AtomicReaderContext context) throws IOException { collector.setNextReader(context); this.docBase = context.docBase; + if (Long.MIN_VALUE == t0) { + setBaseline(); + } } @Override @@ -228,4 +172,131 @@ public class TimeLimitingCollector extends Collector { return collector.acceptsDocsOutOfOrder(); } + + /** + * Returns the global TimerThreads {@link Counter} + *

+ * Invoking this creates may create a new instance of {@link TimerThread} iff + * the global {@link TimerThread} has never been accessed before. The thread + * returned from this method is started on creation and will be alive unless + * you stop the {@link TimerThread} via {@link TimerThread#stopTimer()}. + *

+ * @return the global TimerThreads {@link Counter} + * @lucene.experimental + */ + public static Counter getGlobalCounter() { + return TimerThreadHolder.THREAD.counter; + } + + /** + * Returns the global {@link TimerThread}. + *

+ * Invoking this creates may create a new instance of {@link TimerThread} iff + * the global {@link TimerThread} has never been accessed before. The thread + * returned from this method is started on creation and will be alive unless + * you stop the {@link TimerThread} via {@link TimerThread#stopTimer()}. + *

+ * + * @return the global {@link TimerThread} + * @lucene.experimental + */ + public static TimerThread getGlobalTimerThread() { + return TimerThreadHolder.THREAD; + } + + private static final class TimerThreadHolder { + static final TimerThread THREAD; + static { + THREAD = new TimerThread(Counter.newCounter(true)); + THREAD.start(); + } + } + + /** + * @lucene.experimental + */ + public static final class TimerThread extends Thread { + + public static final String THREAD_NAME = "TimeLimitedCollector timer thread"; + public static final int DEFAULT_RESOLUTION = 20; + // NOTE: we can avoid explicit synchronization here for several reasons: + // * updates to volatile long variables are atomic + // * only single thread modifies this value + // * use of volatile keyword ensures that it does not reside in + // a register, but in main memory (so that changes are visible to + // other threads). + // * visibility of changes does not need to be instantaneous, we can + // afford losing a tick or two. + // + // See section 17 of the Java Language Specification for details. + private volatile long time = 0; + private volatile boolean stop = false; + private volatile long resolution; + final Counter counter; + + public TimerThread(long resolution, Counter counter) { + super(THREAD_NAME); + this.resolution = resolution; + this.counter = counter; + this.setDaemon(true); + } + + public TimerThread(Counter counter) { + this(DEFAULT_RESOLUTION, counter); + } + + @Override + public void run() { + while (!stop) { + // TODO: Use System.nanoTime() when Lucene moves to Java SE 5. + counter.addAndGet(resolution); + try { + Thread.sleep( resolution ); + } catch (InterruptedException ie) { + throw new ThreadInterruptedException(ie); + } + } + } + + /** + * Get the timer value in milliseconds. + */ + public long getMilliseconds() { + return time; + } + + /** + * Stops the timer thread + */ + public void stopTimer() { + stop = true; + } + + /** + * Return the timer resolution. + * @see #setResolution(long) + */ + public long getResolution() { + return resolution; + } + + /** + * Set the timer resolution. + * The default timer resolution is 20 milliseconds. + * This means that a search required to take no longer than + * 800 milliseconds may be stopped after 780 to 820 milliseconds. + *
Note that: + * + */ + public void setResolution(long resolution) { + this.resolution = Math.max(resolution, 5); // 5 milliseconds is about the minimum reasonable time for a Object.wait(long) call. + } + } + } diff --git a/lucene/src/java/org/apache/lucene/util/Counter.java b/lucene/src/java/org/apache/lucene/util/Counter.java index d605aef549e..b7642acb3eb 100644 --- a/lucene/src/java/org/apache/lucene/util/Counter.java +++ b/lucene/src/java/org/apache/lucene/util/Counter.java @@ -77,7 +77,7 @@ public abstract class Counter { } private final static class AtomicCounter extends Counter { - private AtomicLong count; + private final AtomicLong count = new AtomicLong(); @Override public long addAndGet(long delta) { diff --git a/lucene/src/test/org/apache/lucene/search/TestTimeLimitingCollector.java b/lucene/src/test/org/apache/lucene/search/TestTimeLimitingCollector.java index 0a3b1a7129d..38119a244d1 100644 --- a/lucene/src/test/org/apache/lucene/search/TestTimeLimitingCollector.java +++ b/lucene/src/test/org/apache/lucene/search/TestTimeLimitingCollector.java @@ -28,17 +28,17 @@ import org.apache.lucene.index.IndexReader.AtomicReaderContext; import org.apache.lucene.index.RandomIndexWriter; import org.apache.lucene.index.Term; import org.apache.lucene.search.TimeLimitingCollector.TimeExceededException; +import org.apache.lucene.search.TimeLimitingCollector.TimerThread; import org.apache.lucene.store.Directory; +import org.apache.lucene.util.Counter; import org.apache.lucene.util.LuceneTestCase; import org.apache.lucene.util.ThreadInterruptedException; -import org.junit.Ignore; /** * Tests the {@link TimeLimitingCollector}. This test checks (1) search * correctness (regardless of timeout), (2) expected timeout behavior, * and (3) a sanity test with multiple searching threads. */ -@Ignore("broken: see https://issues.apache.org/jira/browse/LUCENE-2822") public class TestTimeLimitingCollector extends LuceneTestCase { private static final int SLOW_DOWN = 3; private static final long TIME_ALLOWED = 17 * SLOW_DOWN; // so searches can find about 17 docs. @@ -57,6 +57,8 @@ public class TestTimeLimitingCollector extends LuceneTestCase { private final String FIELD_NAME = "body"; private Query query; + private Counter counter; + private TimerThread counterThread; /** * initializes searcher with a document set @@ -64,6 +66,9 @@ public class TestTimeLimitingCollector extends LuceneTestCase { @Override public void setUp() throws Exception { super.setUp(); + counter = Counter.newCounter(true); + counterThread = new TimerThread(counter); + counterThread.start(); final String docText[] = { "docThatNeverMatchesSoWeCanRequireLastDocCollectedToBeGreaterThanZero", "one blah three", @@ -98,7 +103,6 @@ public class TestTimeLimitingCollector extends LuceneTestCase { // warm the searcher searcher.search(query, null, 1000); - } @Override @@ -106,6 +110,8 @@ public class TestTimeLimitingCollector extends LuceneTestCase { searcher.close(); reader.close(); directory.close(); + counterThread.stopTimer(); + counterThread.join(); super.tearDown(); } @@ -147,7 +153,7 @@ public class TestTimeLimitingCollector extends LuceneTestCase { } private Collector createTimedCollector(MyHitCollector hc, long timeAllowed, boolean greedy) { - TimeLimitingCollector res = new TimeLimitingCollector(hc, timeAllowed); + TimeLimitingCollector res = new TimeLimitingCollector(hc, counter, timeAllowed); res.setGreedy(greedy); // set to true to make sure at least one doc is collected. return res; } @@ -199,8 +205,8 @@ public class TestTimeLimitingCollector extends LuceneTestCase { // verify that elapsed time at exception is within valid limits assertEquals( timoutException.getTimeAllowed(), TIME_ALLOWED); // a) Not too early - assertTrue ( "elapsed="+timoutException.getTimeElapsed()+" <= (allowed-resolution)="+(TIME_ALLOWED-TimeLimitingCollector.getResolution()), - timoutException.getTimeElapsed() > TIME_ALLOWED-TimeLimitingCollector.getResolution()); + assertTrue ( "elapsed="+timoutException.getTimeElapsed()+" <= (allowed-resolution)="+(TIME_ALLOWED-counterThread.getResolution()), + timoutException.getTimeElapsed() > TIME_ALLOWED-counterThread.getResolution()); // b) Not too late. // This part is problematic in a busy test system, so we just print a warning. // We already verified that a timeout occurred, we just can't be picky about how long it took. @@ -215,7 +221,7 @@ public class TestTimeLimitingCollector extends LuceneTestCase { } private long maxTime(boolean multiThreaded) { - long res = 2 * TimeLimitingCollector.getResolution() + TIME_ALLOWED + SLOW_DOWN; // some slack for less noise in this test + long res = 2 * counterThread.getResolution() + TIME_ALLOWED + SLOW_DOWN; // some slack for less noise in this test if (multiThreaded) { res *= MULTI_THREAD_SLACK; // larger slack } @@ -226,7 +232,7 @@ public class TestTimeLimitingCollector extends LuceneTestCase { String s = "( " + "2*resolution + TIME_ALLOWED + SLOW_DOWN = " + - "2*" + TimeLimitingCollector.getResolution() + " + " + TIME_ALLOWED + " + " + SLOW_DOWN + + "2*" + counterThread.getResolution() + " + " + TIME_ALLOWED + " + " + SLOW_DOWN + ")"; if (multiThreaded) { s = MULTI_THREAD_SLACK + " * "+s; @@ -240,22 +246,22 @@ public class TestTimeLimitingCollector extends LuceneTestCase { public void testModifyResolution() { try { // increase and test - long resolution = 20 * TimeLimitingCollector.DEFAULT_RESOLUTION; //400 - TimeLimitingCollector.setResolution(resolution); - assertEquals(resolution, TimeLimitingCollector.getResolution()); + long resolution = 20 * TimerThread.DEFAULT_RESOLUTION; //400 + counterThread.setResolution(resolution); + assertEquals(resolution, counterThread.getResolution()); doTestTimeout(false,true); // decrease much and test resolution = 5; - TimeLimitingCollector.setResolution(resolution); - assertEquals(resolution, TimeLimitingCollector.getResolution()); + counterThread.setResolution(resolution); + assertEquals(resolution, counterThread.getResolution()); doTestTimeout(false,true); // return to default and test - resolution = TimeLimitingCollector.DEFAULT_RESOLUTION; - TimeLimitingCollector.setResolution(resolution); - assertEquals(resolution, TimeLimitingCollector.getResolution()); + resolution = TimerThread.DEFAULT_RESOLUTION; + counterThread.setResolution(resolution); + assertEquals(resolution, counterThread.getResolution()); doTestTimeout(false,true); } finally { - TimeLimitingCollector.setResolution(TimeLimitingCollector.DEFAULT_RESOLUTION); + counterThread.setResolution(TimerThread.DEFAULT_RESOLUTION); } } diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index 3ce2a98b89f..6c8d7c6728f 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -1294,7 +1294,7 @@ public class SolrIndexSearcher extends IndexSearcher implements SolrInfoMBean { } if( timeAllowed > 0 ) { - collector = new TimeLimitingCollector(collector, timeAllowed); + collector = new TimeLimitingCollector(collector, TimeLimitingCollector.getGlobalCounter(), timeAllowed); } if (pf.postFilter != null) { pf.postFilter.setLastDelegate(collector); @@ -1323,7 +1323,7 @@ public class SolrIndexSearcher extends IndexSearcher implements SolrInfoMBean { } Collector collector = topCollector; if( timeAllowed > 0 ) { - collector = new TimeLimitingCollector(collector, timeAllowed); + collector = new TimeLimitingCollector(collector, TimeLimitingCollector.getGlobalCounter(), timeAllowed); } if (pf.postFilter != null) { pf.postFilter.setLastDelegate(collector); @@ -1413,7 +1413,7 @@ public class SolrIndexSearcher extends IndexSearcher implements SolrInfoMBean { } if( timeAllowed > 0 ) { - collector = new TimeLimitingCollector(collector, timeAllowed); + collector = new TimeLimitingCollector(collector, TimeLimitingCollector.getGlobalCounter(), timeAllowed); } if (pf.postFilter != null) { pf.postFilter.setLastDelegate(collector); @@ -1449,7 +1449,7 @@ public class SolrIndexSearcher extends IndexSearcher implements SolrInfoMBean { Collector collector = setCollector; if( timeAllowed > 0 ) { - collector = new TimeLimitingCollector(collector, timeAllowed ); + collector = new TimeLimitingCollector(collector, TimeLimitingCollector.getGlobalCounter(), timeAllowed ); } if (pf.postFilter != null) { pf.postFilter.setLastDelegate(collector);