Re: [fiji-devel] Status reporting in ImgLib Algorithms

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [fiji-devel] Status reporting in ImgLib Algorithms

Tobias Pietzsch
Hi all,

First of all, I am very reluctant to add any dependency to imglib2-algorithms core.

I admit that I did not really look into scijava-common so far, so I cannot really comment on how severe I think this would be.
I had a quick look and saw that scijava-common is itself not without dependencies. I found it depends on org.bushe.eventbus. On the eventbus website I found the notice:
Update 3/2013: The EventBus has gone stale and is unlikely to be extended to other platforms. Suitable, though more primitive, replacements can be found for GWT (GWT EventBus), Javascript (TIBCO PageBus - an OpenAjax Hub implementation) and Flex (Parsley's Messaging)
So would that mean dragging in a dead project as a transitive dependency?

As far as I can tell, the net.imglib2.multithreading stuff is a leftover from imglib1 times and I think I have mentioned several times that I would like to get rid of it… :-)
I guess that scijava-common ThreadService is more thought-through than the SimpleMultiThreading class. So replacing that I would see as an advantage.
In general, I think ExecutorServices are a good fit for multithreading pixel processing algorithms, along the lines of submitting a runnable for every line/plane/.

I see that you have a need for status reporting in ImageJ2 but I'm not sure that it should be in imglib-algorithms.
I would favour a way where we could keep the imglib2 code focused on processing pixels, not caring about framework issues.
In principle, status reporting can be performed before entering and after leaving a imglib2 "work-unit".
If fine grained reporting is required, then this would mean splitting up the work into smaller units and reporting in between.

In any case, even if we add scijava-common as a dependency, I strongly suggest to also keep non-reporting versions of the algorithms.
If noone is listening to the status reports thats just wasted computation time. I think the overhead will be quite high in some cases.
For instance in your forked ComputeMinMax algorithm you do some work on updating status for every single pixel, which I think is definitely overdoing it.

As a compromise I could image putting an empty protected method report() that is called periodically by the core algorithm implementation.
The JIT would hopefully just optimize that away if you use the core algorithm. Then for the reporting version you would have to extend the core algorithm and override report().

best regards,
Tobias




On Jul 26, 2013, at 5:09 PM, Mark Hiner <[hidden email]> wrote:

Hi all (especially ImgLib2 developers)

  Up until now, when running ImgLib2 algorithms on large datasets in ImageJ2, the UI was basically frozen for the complete compute time (e.g. when computing Min/Max values in the Brightness/Contrast plugin).

  As of this commit UI-based refreshing and event callback methods are now handled on a separate thread, allowing them to send UI updates (e.g. via the StatusService).

  For the moment, to avoid adding a Scijava-common dependency to ImgLib2 algorithms, I forked the ComputeMinMax algorithm in IJ2 to use the StatusService to report computation progress.

  Going forward though, we should agree on a long term solution:

  A) imglib2 needs its own status reporting mechanism (Listeners and Events etc.)
  B) it needs a SciJava context to use the StatusService
  C) we have to fork every core algorithm in ImgLib2 to add StatusService

  If possible I would like to avoid C, but if it becomes a necessity it would be helpful if the algorithms were refactored for extensibility (having protected field accessors, for example).

  I personally am in favor of B because it would limit code repetition and pave the way for further unification (Part of why I completely forked ComputeMinMax was to use the Scijava-common ThreadService, which has some - I think unnecessary - overlap with the SimpleMultiThreading utility class).

  But I understand that adding this dependency is not a minor request, and care would have to be taken to limit the impact on performance.

  So what do other people think?

Thanks,
- Mark

--
--
Please avoid top-posting, and please make sure to reply-to-all!
 
Mailing list web interface: http://groups.google.com/group/fiji-devel
 
---
You received this message because you are subscribed to the Google Groups "Fiji-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/groups/opt_out.
 
 


_______________________________________________
ImageJ-devel mailing list
[hidden email]
http://imagej.net/mailman/listinfo/imagej-devel
Reply | Threaded
Open this post in threaded view
|

Re: [fiji-devel] Status reporting in ImgLib Algorithms

dscho
Administrator
Hi Tobias,

On Mon, 5 Aug 2013, Tobias Pietzsch wrote:

> First of all, I am very reluctant to add any dependency to
> imglib2-algorithms core.

Oh really? ;-)

> I admit that I did not really look into scijava-common so far, so I
> cannot really comment on how severe I think this would be.

I think it might make a lot of sense to look into scijava-common because
it addresses a lot of very general issues with Java programming.

For example, we do often have the problem of inversion of control (IoC):
you will want to let consumers of your code override certain aspects, but
still provide your own, default implementation.

The status service is actually an excellent example for this: lengthy
operations will need to be able to report their progress. For the ImgLib2
developers, stderr is probably not only sufficient, but even desirable.

For most users, stderr is completely inacceptable, especially on Windows
where Java's stderr is re-routed to NUL: by default. Gone. Never to be
seen.

The best way I can think of to address this issue is to have an interface
that describes progress updates, a default implementation writing that to
stderr, and a way to inject different implementations for those clicky
clicky user interfaces.

The way scijava-common addresses this issue is to offer the StatusService
interface:

https://github.com/scijava/scijava-common/blob/master/src/main/java/org/scijava/app/StatusService.java

along with a default implementation:

https://github.com/scijava/scijava-common/blob/master/src/main/java/org/scijava/app/DefaultStatusService.java

And if you want to override it, you can easily do so by implementing the
StatusService, annotating the implementing class with the

        @Plugin(type = StatusService.class)

annotation.

Of course, you are free to reinvent the wheel in ImgLib2 if you so desire,
but please do not ask me to be a fan of that idea.

> I had a quick look and saw that scijava-common is itself not without
> dependencies. I found it depends on org.bushe.eventbus. On the eventbus
> website I found the notice:
>
> Update 3/2013: The EventBus has gone stale and is unlikely to be
> extended to other platforms. Suitable, though more primitive,
> replacements can be found for GWT (GWT EventBus), Javascript (TIBCO
> PageBus - an OpenAjax Hub implementation) and Flex (Parsley's Messaging)
> So would that mean dragging in a dead project as a transitive dependency?

Two points about this:

1) EventBus is very stable and we can easily extend it should we ever need
that. We now even have a Git-SVN mirror for easier debugging, making it
safe to rely on said component:

        https://github.com/scijava/eventbus

2) EventBus is -- as you pointed out -- a *transitive* dependency. No user
of scijava-common needs to use it directly. Actually, no user of
scijava-common needs to use it *full stop*. The point of a
transitive dependency, after all, is that the dependency can switch to any
other way to provide the functionality and your project still runs fine.
And in the case of the eventbus dependency: as I laid out before, the
inversion-of-control principle does even allow you to exclude eventbus.jar
from the classpath completely, given a replacement implementation of the
EventService interface.

> As far as I can tell, the net.imglib2.multithreading stuff is a leftover
> from imglib1 times and I think I have mentioned several times that I
> would like to get rid of it… :-)
>
> I guess that scijava-common ThreadService is more thought-through than
> the SimpleMultiThreading class. So replacing that I would see as an
> advantage.

Indeed, also from the Open Source point of view where you do not only use
software libraries but also contribute improvements back, for a mutual
benefit.

> In general, I think ExecutorServices are a good fit for multithreading
> pixel processing algorithms, along the lines of submitting a runnable
> for every line/plane/.
>
> I see that you have a need for status reporting in ImageJ2 but I'm not
> sure that it should be in imglib-algorithms.

As I said, if you want to implement the very same functionality in
imglib2-algorithms, go ahead. We need that functionality, as do virtually
all developers having any kind of graphical or web frontend.

I would strongly encourage you to save the time, though, and use and
improve what is there in scijava-common.

> I would favour a way where we could keep the imglib2 code focused on
> processing pixels, not caring about framework issues.

Too late. ImgLib2 *is* used, and the users *need* status updates. Any way
you turn it, we need that functionality.

> In principle, status reporting can be performed before entering and
> after leaving a imglib2 "work-unit".

Not without that "work-unit" knowing about the status reporting, because
it needs to report the numbers based on internal state.

> If fine grained reporting is required, then this would mean splitting up
> the work into smaller units and reporting in between.
>
> In any case, even if we add scijava-common as a dependency, I strongly
> suggest to also keep non-reporting versions of the algorithms.

I thought the point of ImgLib2 was to prevent having to duplicate code.

> If noone is listening to the status reports thats just wasted
> computation time. I think the overhead will be quite high in some cases.

As your benchmarks pointed out: if the code path through an interface
always lands at the same implementation, the JIT is quite good at
inlining the code.

So if you override the StatusService with an implementation that has only
empty, final methods, I would be highly surprised if your earlier
benchmarks would be contradicted by the overhead you suspect.

> For instance in your forked ComputeMinMax algorithm you do some work on
> updating status for every single pixel, which I think is definitely
> overdoing it.

That is a problem of the ComputeMinMax algorithm, I agree. But nothing
that can serve as a counterargument against using scijava-common.

> As a compromise I could image putting an empty protected method report()
> that is called periodically by the core algorithm implementation.

But that will be insufficient, don't you think? How would the report()
function know 1) about internal state of the consumer, and 2) about the
percentage of the progress?

> The JIT would hopefully just optimize that away if you use the core
> algorithm. Then for the reporting version you would have to extend the
> core algorithm and override report().

As I pointed out above, the JIT has been consistent in your benchmarks in
inlining short, final methods. It would be more than just surprising if
that would not work for empty methods.

The point is that ImgLib2 should cater for the common case. And the common
case *is* a user interface with some sort of progress report.

If you have a better idea, let's hear it.

Ciao,
Dscho
_______________________________________________
ImageJ-devel mailing list
[hidden email]
http://imagej.net/mailman/listinfo/imagej-devel
Reply | Threaded
Open this post in threaded view
|

Re: [fiji-devel] Status reporting in ImgLib Algorithms

Curtis Rueden
In reply to this post by Tobias Pietzsch
Hi Tobias,

> I had a quick look and saw that scijava-common is itself not without
> dependencies. I found it depends on org.bushe.eventbus.

Yes. SciJava Common also depends on SezPoz. However, both of these dependencies are encapsulated, meaning that no downstream code is "infected" with any direct references to classes from either SezPoz or EventBus. All event logic goes through the SciJava Common EventService. And all SezPoz logic is transparent to the developer, via APT.

> On the eventbus website I found the notice:
> Update 3/2013: The EventBus has gone stale

Thanks for the info; I hadn't seen that. Fortunately, the version of EventBus we use does everything we need. We are unlikely to need upstream support or new versions. At worst, we can fork the project and cut our own new release (and/or integrate the EventBus code into SciJava Common itself) in case a future critical bug is discovered.

> I would favour a way where we could keep the imglib2 code focused on
> processing pixels, not caring about framework issues.

What about Mark's option (A): "imglib2 needs its own status reporting mechanism (Listeners and Events etc.)"?

> I think the overhead will be quite high in some cases. For instance in
> your forked ComputeMinMax algorithm you do some work on updating
> status for every single pixel, which I think is definitely overdoing
> it.

Yes, there is always a tradeoff, and a balance, for this problem. I agree that status-per-pixel is overkill.

What about polling? Another option might be for ImgLib2 algorithms to update some (standardized) internal state, but not inform anyone about such changes. Then downstream code could poll that state every X milliseconds.

Regards,
Curtis


On Mon, Aug 5, 2013 at 11:01 AM, Tobias Pietzsch <[hidden email]> wrote:
Hi all,

First of all, I am very reluctant to add any dependency to imglib2-algorithms core.

I admit that I did not really look into scijava-common so far, so I cannot really comment on how severe I think this would be.
I had a quick look and saw that scijava-common is itself not without dependencies. I found it depends on org.bushe.eventbus. On the eventbus website I found the notice:
Update 3/2013: The EventBus has gone stale and is unlikely to be extended to other platforms. Suitable, though more primitive, replacements can be found for GWT (GWT EventBus), Javascript (TIBCO PageBus - an OpenAjax Hub implementation) and Flex (Parsley's Messaging)
So would that mean dragging in a dead project as a transitive dependency?

As far as I can tell, the net.imglib2.multithreading stuff is a leftover from imglib1 times and I think I have mentioned several times that I would like to get rid of it… :-)
I guess that scijava-common ThreadService is more thought-through than the SimpleMultiThreading class. So replacing that I would see as an advantage.
In general, I think ExecutorServices are a good fit for multithreading pixel processing algorithms, along the lines of submitting a runnable for every line/plane/.

I see that you have a need for status reporting in ImageJ2 but I'm not sure that it should be in imglib-algorithms.
I would favour a way where we could keep the imglib2 code focused on processing pixels, not caring about framework issues.
In principle, status reporting can be performed before entering and after leaving a imglib2 "work-unit".
If fine grained reporting is required, then this would mean splitting up the work into smaller units and reporting in between.

In any case, even if we add scijava-common as a dependency, I strongly suggest to also keep non-reporting versions of the algorithms.
If noone is listening to the status reports thats just wasted computation time. I think the overhead will be quite high in some cases.
For instance in your forked ComputeMinMax algorithm you do some work on updating status for every single pixel, which I think is definitely overdoing it.

As a compromise I could image putting an empty protected method report() that is called periodically by the core algorithm implementation.
The JIT would hopefully just optimize that away if you use the core algorithm. Then for the reporting version you would have to extend the core algorithm and override report().

best regards,
Tobias




On Jul 26, 2013, at 5:09 PM, Mark Hiner <[hidden email]> wrote:

Hi all (especially ImgLib2 developers)

  Up until now, when running ImgLib2 algorithms on large datasets in ImageJ2, the UI was basically frozen for the complete compute time (e.g. when computing Min/Max values in the Brightness/Contrast plugin).

  As of this commit UI-based refreshing and event callback methods are now handled on a separate thread, allowing them to send UI updates (e.g. via the StatusService).

  For the moment, to avoid adding a Scijava-common dependency to ImgLib2 algorithms, I forked the ComputeMinMax algorithm in IJ2 to use the StatusService to report computation progress.

  Going forward though, we should agree on a long term solution:

  A) imglib2 needs its own status reporting mechanism (Listeners and Events etc.)
  B) it needs a SciJava context to use the StatusService
  C) we have to fork every core algorithm in ImgLib2 to add StatusService

  If possible I would like to avoid C, but if it becomes a necessity it would be helpful if the algorithms were refactored for extensibility (having protected field accessors, for example).

  I personally am in favor of B because it would limit code repetition and pave the way for further unification (Part of why I completely forked ComputeMinMax was to use the Scijava-common ThreadService, which has some - I think unnecessary - overlap with the SimpleMultiThreading utility class).

  But I understand that adding this dependency is not a minor request, and care would have to be taken to limit the impact on performance.

  So what do other people think?

Thanks,
- Mark

--
--
Please avoid top-posting, and please make sure to reply-to-all!
 
Mailing list web interface: http://groups.google.com/group/fiji-devel
 
---
You received this message because you are subscribed to the Google Groups "Fiji-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/groups/opt_out.
 
 

--
--
Please avoid top-posting, and please make sure to reply-to-all!
 
Mailing list web interface: http://groups.google.com/group/fiji-devel
 
---
You received this message because you are subscribed to the Google Groups "Fiji-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/groups/opt_out.
 
 


_______________________________________________
ImageJ-devel mailing list
[hidden email]
http://imagej.net/mailman/listinfo/imagej-devel
Reply | Threaded
Open this post in threaded view
|

Re: [fiji-devel] Status reporting in ImgLib Algorithms

dscho
Administrator
Hi Curtis,

On Mon, 5 Aug 2013, Curtis Rueden wrote:

> On Mon, Aug 5, 2013 at 11:01 AM, Tobias Pietzsch
> <[hidden email]>wrote:
>
> > I think the overhead will be quite high in some cases. For instance in
> > your forked ComputeMinMax algorithm you do some work on updating
> > status for every single pixel, which I think is definitely overdoing
> > it.
>
> Yes, there is always a tradeoff, and a balance, for this problem. I
> agree that status-per-pixel is overkill.
>
> What about polling? Another option might be for ImgLib2 algorithms to
> update some (standardized) internal state, but not inform anyone about
> such changes. Then downstream code could poll that state every X
> milliseconds.

I agree that there is a danger of overkill. Unfortunately, the same danger
applies to the core library trying to think for the consumers.

A concrete example how things can be decoupled can be found in ImageJ 1.x:
the algorithms typically update their prograss on a slice-by-slice basis,
or if the algorithm is known to be expensive, pixel line by pixel line. Of
course, for small images this still meant that the progress updates would
dominate the execution time (undesirable, of course).

ImageJ 1.x deals with it by having its own internal state in the progress
update that skips progress updates if they come in too fast.

Likewise, I would suggest that ImgLib2 does not try to think for the
status reporter. Instead, there should be a way to report the status. Full
stop. The status reporter consuming those calls would need to do something
IJ1-like if there is a performance problem.

Let's separate concerns properly,
Dscho

_______________________________________________
ImageJ-devel mailing list
[hidden email]
http://imagej.net/mailman/listinfo/imagej-devel