Re: Multithreaded FFTW3 in octave?

[Mike Miller]

On Mon, Dec 17, 2012 at 1:04 PM, Jordi Gutiérrez Hermoso
<address@hidden> wrote:
> On 16 December 2012 19:05, Andreas Weber <address@hidden> wrote:
>> Hi Mike, I've attached a "first shot" patch which
>
> I like this patch. At a glance, it looks like a very good addition.
> I'll wait for Mike's comments. If he doesn't merge it in within a
> couple of days, I will.

Looks good overall, thanks for working on this. I just tested briefly
and it works for me as expected, for the most part. I do have a few
comments.

The configure changes don't look quite right to me. I wouldn't make
the HAVE_FFTW3_THREADS condition dependent on a separate fftw3_threads
option. The way you have it here I don't think I can configure
"--with-fftw3=somefftwlib" and have it detect whether some
FFTW-compatible library has the threading API or not. It's also
linking with both fftw3_threads and fftw3 (non-threaded). I'd rather
see it test the existing FFTW3_LIBS and FFTW3F_LIBS whether they
implement one of the threading functions.

I'm not sure off the top of my head what's the cleanest way to do it
that way *and* make it automatically use the libfftw3_threads library
if it's installed. And how to express whether to use libfftw3_threads
or libfftw3_omp? Worst case, we could require users to configure with
"--with-fftw3=fftw3_threads --with-fftw3f=fftw3f_threads", this has
pros and cons. I think I prefer that for now, it's the easiest and
safest and we can always change it later.

On to the code...

I would set the default number of threads to 1 at startup. Matches the
default behavior of both the non-threaded and threaded FFTW libraries.

There is some trailing whitespace on a few lines that should be removed.

I already fixed the method / do_method bug recently on the stable
branch (http://hg.savannah.gnu.org/hgweb/octave/rev/f63a4f23bfe7), I
think it simply hasn't been merged into the dev branch yet, so don't
include that in this patch.

Other than that, looks real good. Rebuttal or does my feedback make
sense? Do you want to work on cleaning up some of these things and
submit another patch?

-- 
mike