On 17/02/11 20:25, Jeffrey Ollie wrote:
I think that I've tracked down an NPE in the crosby_integration branch
of the splitter. The SplitProcessor is using a BlockingQueue to queue
up elements for writing. The poll() method can potentially return a
null if the queue is empty. My question is, should the poll() method
call be switched to a take() method call - the take() method will
block if the queue is empty, or should I guard the usage of elements
for null like in the patch below?
I think that if you get a null then you should break out of the loop
so that the thread can pick up the next work package. By waiting or
spinning while the workPackage is empty, the thread wastes time (and I
don't suppose there is any guarantee that anything will ever be added).
But lets look at what is happening, this is my take:
The synchronized line does nothing useful so it can be removed.
Then there is a check that the queue is not empty.
Later we try to read from it and find that it is empty. This means
that some other thread is also reading from the same queue. From the
code this is possible, but possibly unintended - I would expect that
the reason that the work is parcelled up in the first place is to
avoid excessive contention reading work items by different threads
from a single queue.
In any case, the check for empty is also useless and so can be
removed. The remaining code would then look like this:
ArrayList<Element> elements;
while ((elements = workPackage.inputQueue.poll()) != null) {
try {
for (Element element : elements ) {
processElement(element, workPackage.writer);
}
} catch (IOException e) {
throw new RuntimeException("Thread " +
Thread.currentThread().getName() + " failed to write element ", e);
}
}
Also attached as a patch, could you try it please.
Oh and thanks for looking into these problems, I'm going to look
at your second patch now.
Best wishes
..Steve
Index: src/uk/me/parabola/splitter/SplitProcessor.java
===================================================================
--- src/uk/me/parabola/splitter/SplitProcessor.java (revision 160)
+++ src/uk/me/parabola/splitter/SplitProcessor.java (revision )
@@ -12,6 +12,8 @@
*/
package uk.me.parabola.splitter;
+import uk.me.parabola.splitter.Relation.Member;
+
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
@@ -23,8 +25,6 @@
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
-import uk.me.parabola.splitter.Relation.Member;
-
/**
* Splits a map into multiple areas.
*/
@@ -318,20 +318,16 @@
}
finished=true;
} else {
- synchronized (workPackage) {
- while (!workPackage.inputQueue.isEmpty()) {
- ArrayList<Element> elements =null;
+ ArrayList<Element> elements;
+ while ((elements = workPackage.inputQueue.poll()) != null) {
try {
- elements = workPackage.inputQueue.poll();
for (Element element : elements ) {
processElement(element, workPackage.writer);
}
-
} catch (IOException e) {
throw new RuntimeException("Thread " + Thread.currentThread().getName() + " failed to write element ", e);
}
}
- }
}
}
_______________________________________________
mkgmap-dev mailing list
mkgmap-dev@lists.mkgmap.org.uk
http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
