Re: Feedback from GSL folks on libflame 4.0

["Field G. Van Zee" <field at cs dot utexas dot edu>]

Gerard,

Thanks for taking the time to review our library! I'll try to respond to each item.

At Tue, 16 Feb 2010 16:37:31 -0700, Gerard Jungman wrote:
> 
> I spent a little time looking at the libflame manual. I have some
> questions; maybe somebody knows the answers.
> 
> (1) The library requires FLA_Init() and FLA_Finalize(). I wonder
>    what library-level data is initialized and how this interacts
>    with multi-threading. It looks like it is meant to work ok
>    with threads, but there is a build switch (--enable-multithreading)
>    which makes me wonder what is going on. I don't like
>    any kind of hidden state, and I really don't like
>    hidden global state.

The bulk of the library-level data that is initialized consists of the global
scalar constants, FLA_ONE, FLA_ZERO, etc. We also have a memory leak counter
that counts the number of mallocs and frees that is initialized, as well as an
array of error messages and some other internal data structures.

How to implement the scalars has always been somewhat of a pain. Very early on
we decided on what we thought was a "clean" implementation where the constants
are actually 1x1 FLA_Objs that were initialized up front. This is nice since it
makes these scalars indistinguishable from user-created 1x1's. Thus, we can
create interfaces for routines that, say, scale an object by a scalar and let
the user invoke the routine with either a pre-defined scalar or one that he
created on his own if he needs a different value. So from a type perspective,
this is very clean. The problem is we would like those scalars (FLA_ONE,
FLA_ZERO, etc.) to already exist in the current scope. The most obvious way to
do this was to park them in the global address space and initialize them in
FLA_Init(). We acknowledge that this is not a great solution since it results in
hidden global state. I would be open to changing the library if we can come up
with an alternative that is workable.

The --enable-multithreading option enables multithreading within the library
only. It does not change the friendliness of the library routines to being
invoked from multiple threads created outside of libflame. (I'm not sure if
libflame would work in such a situation.) Granted, this is not what we want, but
making the library thread-safe/reentrant/etc, has not been a high priority in
the past.

> 
> (2) The LAPACK-like coverage seems reasonable. But I am not a good
>    judge of this. How much LAPACK functionality is covered in
>    this latest release? Obviously all the banded-matrix stuff
>    is out, since libflame does nothing with banded matrices.
>    But how complete is it regarding core functionality?

As Brian mentioned, SVD and eigenvalue decomposition routines are missing. We
are considering providing object-based FLAME interfaces to them for those who
are going to use LAPACK anyway. We also have plans to add native implementations
of EVD/SVD in the future, but that is probably still 6-9 months away.

> 
> (3) The separation of the metadata (FLA_Obj) and the data buffer is
>    good. At least this means that a wrapper implementation (say C++)
>    can use any allocation scheme it likes for the buffer. I'm not sure
>    about FLA_Obj itself. From the code examples, it appears that
>    FLA_Obj is stack friendly. But I can't be sure without looking
>    at the headers. So I guess I can answer this question myself...
>    but I'm tired now.

Could you clarify what you mean by "stack friendly"? We declare FLA_Obj's on the
stack, but they still need to be initialized and have data buffers "attached" to
them. Both of these things happen when the object is created via one of the
FLA_Obj_create_*() routines.

> 
> (4) According to the manual, libflame calls abort() when it encounters
>    a problem. As I have discussed before, this is brain-damaged. It
>    makes it hard for other library developers (us) to integrate
>    their thing into an existing error-handling system. They seem
>    to admit it is a problem, but it's probably a low priority
>    for them. How can we integrate this?

I'm not going to argue with you here; it is brain-damaged. However, early on,
when libflame was still very much under heavy development and only used by its
developers, it made sense to abort() when something was wrong. That is to say,
for example, if there were any preconditions to a function that were not met,
proceeding would (probably) result in undefined behavior. It seemed like a
better idea to just get our attention so we could fix the problem. We agree that
using return values would be a more standard way of handling errors, but we're
also somewhat cynical in that we don't trust our users to check return values.

Bottom line: we are anything but married to the idea of aborting when an error
is encountered, but we are unsure how to come up with a solution that is less
brain-damaged and portable and that fits our users' coding style. It would be
nice to be able to throw exceptions, but of course C has no such mechanism.

> 
> (5) There are many configuration/build options. Is it feasible to
>    build and deploy several different versions (with and without
>    SuperMatrix, etc), from which a selection can be made at link
>    time, requiring no source-level changes in client code?

Many of the options don't need to be tweaked, and/or can be "fixed" to their
more portable or more general setting. As for choosing a different build at
link-time, I can't really offer much in the way of comment there.

> 
> 
> Some random comments:
> 
> (a) I'm not sure what it would mean to "use libflame under gsl-2.0",
>    as mentioned below. We need to think about ways to insulate
>    ourselves from any specific project, while still allowing it
>    to be used transparently.
> 
> (b) libflame does not provide BLAS functionality, it requires
>    an external BLAS. This is good, since people want to use
>    special optimized versions for their architectures. But
>    it also means we have the same problem as with GSL:
>    detecting and linking against an appropriate BLAS,
>    and dealing with its possible absence. It would be
>    better if we could eliminate gslcblas once and for all,
>    or at least factor it out of the main release somehow.

We have tentative plans to integrate a BLAS implementation at some point in the
future. At that point, libflame will be a complete solution that does not depend
on any external numerical library (aside from m). But yes, for now, we require
external BLAS.

> 
> (c) In the libflame world, it looks like scalars are themselves
>    instances of 'FLA_Obj'. Ok, I can see the logical coherence
>    in this, but it seems like it could be very inconvenient at
>    times.

See above discussion of (1). If we were ever to remove the global scalars, we
might need to replace them with preprocessor macros or perform some other kind
of magic. Alternatively, we could create and free these scalar objects every
time we needed them, though I cringe at the idea of implementing that change.
Any suggestions?

> 
> (d) There are several places where the API assumes C stdio. It looks
>    like some of these uses are internal (like FLA_Print_message
>    being used for error messages). This is brain-damaged, since
>    it makes it harder to integrate into other environments
>    (i.e. C++) where C stdio is not appropriate. It's ok to
>    have such "convenience" functions in the API, but they
>    should not be used internally.

Please suggest a fix and we'll be happy to look into it!

> 
> (e) The autotools build looks somewhat annoying. I'm really
>    tired of autotools. Obviously, the same is true of GSL.

Notice that we only use autoconf, not automake or libtool. Why is using autoconf
undesirable? We were trying to be good GNU software citizens when we designed
the build system.

> 
> (f) The API has both capitals and underscores, the worst choice
>    of all. Seemingly trivial, but it makes me queasy. Will people
>    never learn?

The naming conventions were settled upon before I joined the group. I suspect we
were trying to mimic MPI and other such libraries. It's not exactly my idea
naming convention. I prefer all lower-case with underscores. Who knows, maybe we
will change it at some point in the future?

> 
> 
> I'm not trying to be super-critical. But if we are seriously
> considering this thing, then no stone will be left un-turned.

Don't worry about appearing critical. We appreciate all your feedback! Hopefully
we can make improvements on our side that will make your life easier.

Field