Re: Feedback from GSL folks on libflame 4.0

[Gerard Jungman <jungman at lanl dot gov>]

On Thu, 2010-02-18 at 13:10 -0600, Field G. Van Zee wrote:

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

Hi. This is great, someone to talk to! Thanks very much
for replying. Here are my follow-on comments.


> 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.

It might be re-entrant anyway, or so close to it that only minor changes
would be needed. The global constants are not a problem, in principle.
The memory leak counter may have to be initialized on a per-thread
basis, if that makes sense, similar to the way errno is made
thread-safe in gcc. Brian might be able to look into this,
since he understands how errno works. The trick would be doing
it in a platform-agnostic way, if not truly platform-independent.

In the final picture, I think thread-safety is really important.
More and more, it's going to become a requirement for new code.
Of course, people will get by however they can, but it would be
nice if libflame just worked for them, in those cases where some
linear algebra operation solves their problem.


> We also have plans to add native implementations
> of EVD/SVD in the future, but that is probably still 6-9 months away.

That sounds fine. After all, some of us have been
waiting a decade already. We can wait another year.


> 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.

I have looked at the code now. The "void* buffer" element of
FLA_Base_Obj is not a problem, since it can be attached by
the client to anything appropriate, as you say. In fact, FLA_Base_Obj
is stack-friendly in the sense that I mean, as it does not require
any heap operations to put it into a well-defined state. However, it
seems that FLA_Obj is not. Of course, this is not an absolute
show-stopper, but it's still a bit sticky, since it touches on your
object model. FLA_Obj_create() eventually does a heap allocation to
assign the "FLA_Base_obj* base" member of FLA_Obj. From my point
of view, this heap allocation is gratuitous.

I don't think this is a trivial point, because simple workarounds
(like providing another way to attach "FLA_Base_obj* base" might
open holes in the safety of the whole system. One way is
to use a kind of composition model, where the view carries
FLA_base_Obj directly, rather than by pointer reference.
But this requires rethinking your object model. Also,
the simple question of memory overhead arises, in the
case when many views to a small number of underlying
objects are created.

We had this same discussion on the GSL list recently, regarding
a possible re-design of the GLS containers. The current GSL
containers have the same problem (along with other problems!).
I tried hard there to make the object/view paradigm work
well without large overheads. Of course, that discussion
is still in flux.


> 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.

I agree with the cynicism. GSL tried to handle this by providing
"natural" versions of functions (which act like yours, aborting
when they give up) and "return-code" versions which are meant
for people who want them, and especially for external library
designers who want to use GSL components but need to handle
errors in their own way. This includes situations like C++
library designers who want to wrap GSL but want to throw
exceptions, etc.

The "natural" versions defer to the "return-code" versions
in the obvious way. Of course, this is only one possible
solution. There are others, up to and including a full-blown
callback-based system (which I am not advocating).


> 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.

I would seriously consider the two-prototype solution. I don't think
we have discovered any real problems with it. It increases the size
of the intellectual real-estate occupied by the API, but people
can ignore the parts they don't need.


> 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.

Sure. This was not meant so much for you, as for GSL folks, asking
how we would support variability in this configuration layer, when
using libflame through some link-time selection process. The
desire is that the variations could be handled without
client-side source-code changes. I assume and hope that
this is possible.


> 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.

Right. This will be a good thing eventually. Whatever you produce
will end up being much better than the gslcblas implementation,
which I basically copied from the CBLAS draft standard, oh so
many years ago. Of course, people will still want to use
(and should want to use) their favorite optimized BLAS,
but we could eliminate an entire chunk of annoying
code from GSL.


> 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?

I probably can't suggest anything that you haven't already thought of.
And I can't really judge without having tried to use the stuff. But I
do wonder if the logical coherence of the "scalar = 1x1 object" choice
is not offset by the possible client inconvenience.

And that's not to say that there isn't some magical solution waiting
to be found. But I don't know one off hand.


> > (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!

With regard to the error messages, that will be fixed by whatever
error-handling solution is chosen. Printing the error messages
only makes sense for the "abort" versions of functions anyway.

I don't know if FLA_Print_message() is used anywhere else.
Maybe for "informational" or "warning" messages now and then,
but this is also handled by whatever solution is chosen for
error-handling. I can't imagine C stdio appears anyplace
else. Is that right?


> 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.

I think I'm glad that automake and libtool are not being used. I
guess that I'm mostly agnostic about autoconf on its own. Others
may have specific complaints that they want to air. By the way,
how do you build shared libraries?

This comment is partly my way of jabbing at Brian regarding the GSL
build system. These days I am following a new herd and converting all
my own projects to cmake. Ten years from now I will probably be
complaining about cmake.


> 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?

Yup. That's my preference too, all lower-case with underscores.


> 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.

And I very much appreciate your responses. Feel free to straighten
out any misconceptions I have. All these comments are based on
an afternoon examining the manual and some of the code, and not
real usage, so they should be taken as highly provisional.

Thanks again!
--
G. Jungman