search

imagej / imagej-ops

ImageJ Ops: "Write once, run anywhere" image processing
https://imagej.net/libs/imagej-ops
BSD 2-Clause "Simplified" License
88 stars 42 forks source link

Kill Zombies from the Fractal Dimension #638

Closed mdoube closed 3 years ago

mdoube commented 3 years ago

Fixes #637 ExecutorService thread pools were being created but not cleanly shutdown(). Now they are properly terminated. Happy Friday 13th!

mdoube commented 3 years ago

Closes https://github.com/bonej-org/BoneJ2/issues/300

ctrueden commented 3 years ago

Wow, nice! Thanks. :beers:

I'm wondering why these ops use their own ExecutorServices rather than the one already baked into the ThreadService. But since they do, doing the cleanup as well is definitely a step forward. The new incarnation of ImageJ Ops will use ImgLib2's unified multithreading support.

ctrueden commented 3 years ago

I tried to cut a new release, but am seeing some test failures on my local system (but not on CI):

[INFO] [ERROR] Failures: 
[INFO] [ERROR]   OutlineTest.testEdgeSquare:177 Wrong number of foreground elements in interval expected:<7> but was:<5>
[INFO] [ERROR]   OutlineTest.testEdgeSquareExcludeEdgesFalse:206 Wrong number of foreground elements in interval expected:<8> but was:<6>
[INFO] [ERROR]   OutlineTest.testOutlineSquare:148 Wrong number of foreground elements in interval expected:<8> but was:<5>

I'll investigate when I can.

mdoube commented 3 years ago

test failures on my local system (but not on CI)

This smells like something we've seen for a while. Test failures on some machines (usually bigger ones, e.g. workstations) but passing on CI and smaller machines (e.g. laptops).

https://github.com/bonej-org/BoneJ2/issues/196#issuecomment-627364697 https://github.com/bonej-org/BoneJ2/issues/229

I wonder if there is a problem with the parallelisation model that leads to overwriting of pixel values depending on number of threads in use.

mdoube commented 3 years ago

I tried to cut a new release, but am seeing some test failures on my local system (but not on CI):

[INFO] [ERROR] Failures: 
[INFO] [ERROR]   OutlineTest.testEdgeSquare:177 Wrong number of foreground elements in interval expected:<7> but was:<5>
[INFO] [ERROR]   OutlineTest.testEdgeSquareExcludeEdgesFalse:206 Wrong number of foreground elements in interval expected:<8> but was:<6>
[INFO] [ERROR]   OutlineTest.testOutlineSquare:148 Wrong number of foreground elements in interval expected:<8> but was:<5>

I'll investigate when I can.

This PR fixes it at the cost of running in a single thread, which is OK for now. https://github.com/imagej/imagej-ops/pull/639

mdoube commented 3 years ago

why these ops use their own ExecutorServices rather than the one already baked into the ThreadService

There is another PR from last year that does exactly that, ~~and which should be used instead of my hacks on Outline. https://github.com/imagej/imagej-ops/pull/624~~

but unfortunately that one is buggy and we should use #639 until we figure out a better multithreading strategy. Test and builds now pass and user operation works on my 40-core workstation.

rimadoma commented 3 years ago

I chose to use my own ExecutorService instead of ThreadService because at the time using a FixedThreadPool ran faster with a certain N than what ThreadService used