Re: Feedback from GSL folks on libflame 4.0

Re: Feedback from GSL folks on libflame 4.0

[Rhys Ulerich]

Hi Gerard,

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

I'm willing to take a stab at providing the necessary error handling
underneath flame to make it play nice with GSL's error handling model.
 The obvious choice is to adopt GSL's routines.  However, flame is
LGPL and so adopting GSL's GPLed error handling directly is not an
option.

Do you know of an LGPL project that you believe does a good job in this regard?

- Rhys

Re: Feedback from GSL folks on libflame 4.0

[Gerard Jungman]

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

Re: Feedback from GSL folks on libflame 4.0

[Gerard Jungman]

On Thu, 2010-02-18 at 13:50 -0600, Rhys Ulerich wrote:

> I'm willing to take a stab at providing the necessary error handling
> underneath flame to make it play nice with GSL's error handling model.
> The obvious choice is to adopt GSL's routines.  However, flame is
> LGPL and so adopting GSL's GPLed error handling directly is not an
> option.
> 
> Do you know of an LGPL project that you believe does a good job in this regard?

I don't know of another project, but I haven't looked very hard
(and certainly not in a long time). Anyway, I think that copying
the GSL stuff (if you really want to) only requires asking us.
Brian probably has the copyright for the error-handling stuff
and can just re-license and gift that part of the code (unless
he has some objection or I am missing something).

In my follow-on comments to Field, I suggested the two-prototype model
used by (at least parts of) GSL, where each function has both a
"natural" abort-on-trouble version and a "return-code" version.
The "natural" version defers to the "return-code" version in
the obvious way. I am happy enough with this part of GSL, anyway.
Anything more complicated should probably be discussed critically.
I don't think I'm totally happy with every aspect of GSL error-handling,
mainly because I don't think it was implemented consistently across
the whole library.

The work required is obviously very easy, just a
bit tedious. Eventually the "natural" versions could be
automatically generated, if that seems worth doing. A
catalog of return codes and associated static error
messages is needed. That might require eye-balling
the libflame source, possibly doing some classification
work, depending on how organized they were about their
error conditions.


If you can think of something better, that would be great too.
Any ideas for improvement are welcome.

--
G. Jungman

Re: Feedback from GSL folks on libflame 4.0

[Field G. Van Zee]

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

Re: Feedback from GSL folks on libflame 4.0

[Brian Gough]

At Thu, 18 Feb 2010 13:10:45 -0600,
Field G. Van Zee wrote:
> 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.

Thanks for using autoconf. I think it is a good thing.

As a maintainer, I also like automake and libtool because they
standardise the build process and reduce the amount of code I am
responsible for -- if there is a problem, I just pass it over to the
automake/libtool maintainers.  Automake's "make distcheck" is also a
nice feature.

-- 
Brian Gough

GNU Scientific Library -
http://www.gnu.org/software/gsl/

Re: Feedback from GSL folks on libflame 4.0

[Robert G. Brown]

On Fri, 19 Feb 2010, Brian Gough wrote:

> At Thu, 18 Feb 2010 13:10:45 -0600,
> Field G. Van Zee wrote:
>> 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.
>
> Thanks for using autoconf. I think it is a good thing.
>
> As a maintainer, I also like automake and libtool because they
> standardise the build process and reduce the amount of code I am
> responsible for -- if there is a problem, I just pass it over to the
> automake/libtool maintainers.  Automake's "make distcheck" is also a
> nice feature.

Yeah, I don't really think of them as optional these days.  They are far
from perfect and often a PITA, but what they do is necessary.  And
somebody takes care of them, so they usually aren't broken.  The
problems they solve are ugly problems; one can't expect an absolutely
gorgeous solution.

Kind of a love/hate thing...

    rgb

>
> -- 
> Brian Gough
>
> GNU Scientific Library -
> http://www.gnu.org/software/gsl/
>

Robert G. Brown	                       http://www.phy.duke.edu/~rgb/
Duke University Dept. of Physics, Box 90305
Durham, N.C. 27708-0305
Phone: 1-919-660-2567  Fax: 919-660-2525     email:rgb@phy.duke.edu

Re: Feedback from GSL folks on libflame 4.0

[Brian Gough]

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

The approach in GSL is to have an abort() by default, for the same
reason.  But the user can turn off the abort() and use the error
return values instead by providing an alternative error handler as a
function pointer.  Here's how it would look with your functions:

=== FLA_macro_defs.h ===

// Error code translation and output macro definition.
#define FLA_Check_error_code( code ) \
  do { \
       if (code != FLA_SUCCESS) {  \
         FLA_Check_error_code_helper( code, __FILE__, __LINE__ ); \
         return code; \    /* see comment below about void functions */
       } \
  } while (0)


=== FLA_main_prototypes.h ===

typedef void FLA_error_handler_t (int code, const char * file, int line);


=== FLA_Check.c ===

static FLA_error_handler_t * FLA_error_handler = NULL;

void FLA_Check_error_code_helper( int code, char* file, int line )
{
  if ( code == FLA_SUCCESS )
    return;

  if (FLA_error_handler) 
    {
      (*FLA_error_handler) (code, file, line);
      return ;
    }

  if ( FLA_ERROR_CODE_MAX <= code && code <= FLA_ERROR_CODE_MIN )
  {
    FLA_Print_message( FLA_Error_string_for_code( code ),
                       file, line );
    FLA_Abort();
  }
  else
  {
    FLA_Print_message( FLA_Error_string_for_code( FLA_UNDEFINED_ERROR_CODE ),
                       file, line );
    FLA_Abort();
  }
}


(We actually have three macro GSL_ERROR, GSL_ERROR_VOID and
GSL_ERROR_VAL to handle different types of return value - error code,
void, and "other".  This is to avoid warnings about "returning a value
from a void function" etc).

Re: Feedback from GSL folks on libflame 4.0

[Brian Gough]

At Thu, 18 Feb 2010 13:50:44 -0600,
Rhys Ulerich wrote:
> I'm willing to take a stab at providing the necessary error handling
> underneath flame to make it play nice with GSL's error handling
> model.
>  The obvious choice is to adopt GSL's routines.  However, flame is
> LGPL and so adopting GSL's GPLed error handling directly is not an
> option.
> 
> Do you know of an LGPL project that you believe does a good job in
> this regard?

I think the amount of code is so small that the licensing shouldn't be
a concern here.  If anyone wants to use the error macro definitions or
error handler from GSL under the LGPL I am fine with that.

-- 
Brian Gough

Re: Feedback from GSL folks on libflame 4.0

[Gerard Jungman]

On Fri, 2010-02-19 at 19:40 +0000, Brian Gough wrote:
> 
> The approach in GSL is to have an abort() by default, for the same
> reason.  But the user can turn off the abort() and use the error
> return values instead by providing an alternative error handler as a
> function pointer.  Here's how it would look with your functions:

Yes. But don't do it this way. This is not layered properly.
Read my previous post.

The problem with this is that a third-party library developer
has to figure out how to turn it off. It's better to publish
two interfaces, one "natural" targeted to end-users and one
"return-code" targeted for use by developers of other systems.

--
G. Jungman

Re: Feedback from GSL folks on libflame 4.0

[Gerard Jungman]

On Fri, 2010-02-19 at 13:41 -0500, Robert G. Brown wrote:
> On Fri, 19 Feb 2010, Brian Gough wrote:
> >
> > Thanks for using autoconf. I think it is a good thing.
> 
> Yeah, I don't really think of them as optional these days.  They are far
> from perfect and often a PITA, but what they do is necessary.

You guys are missing the point. The question is not
"autotools or nothing" but "autotools or cmake".

--
G. Jungman

Re: Feedback from GSL folks on libflame 4.0

[Robert G. Brown]

On Fri, 19 Feb 2010, Gerard Jungman wrote:

> On Fri, 2010-02-19 at 13:41 -0500, Robert G. Brown wrote:
>> On Fri, 19 Feb 2010, Brian Gough wrote:
>>>
>>> Thanks for using autoconf. I think it is a good thing.
>>
>> Yeah, I don't really think of them as optional these days.  They are far
>> from perfect and often a PITA, but what they do is necessary.
>
> You guys are missing the point. The question is not
> "autotools or nothing" but "autotools or cmake".

Sure, or (God help us) aimk or other ancient solutions to the problem.
But is cmake any better?  When I tried it last -- some years ago, I
admit -- it wasn't, at least not for me.  If you build a package
intended to go into many different distros (ones with toplevel
referees), they all but require autotools.  Will they accept cmake
instead?

Ultimately, it comes down to standard practice.  Nearly every packaged
application in nearly every linuxoid or BSDish OS uses GBT.  Everybody
knows to cd into the toplevel directory, run ./configure --whatever,
make, and you're done.  Tools like glade build a GBT project directory
for you whether you like it or not.  I tried fighting this for years
more because the documentation still pretty much sucks so you have to
work quite hard to figure out how to actually use the tools and move
past simple templates, but once you get a COMPLEX template set up, sheer
inertia keeps you there.

So, will Debian accept a cmake packaging?  Fedora?  I really don't know,
but when I looked at the Fedora packaging requirements and worked on
getting a package into Debian, the nominal rules called for GBT and
there was a lot of pressure to stick with them because everybody knows
how to build with them.

    rgb

>
> --
> G. Jungman
>
>

Robert G. Brown	                       http://www.phy.duke.edu/~rgb/
Duke University Dept. of Physics, Box 90305
Durham, N.C. 27708-0305
Phone: 1-919-660-2567  Fax: 919-660-2525     email:rgb@phy.duke.edu

Re: Feedback from GSL folks on libflame 4.0

[Gerard Jungman]

On Fri, 2010-02-19 at 16:46 -0500, Robert G. Brown wrote:

> So, will Debian accept a cmake packaging?  Fedora?  I really don't know,

ummm... KDE builds with cmake. Nuff said.

http://lwn.net/Articles/188693/


Boost will eventually build with cmake. It basically
works, but they haven't switched yet.


Here is a list of some set of projects using cmake.

http://www.itk.org/Wiki/CMake_Projects

Highlights:
  KDE, ITK VTK and Paraview, Scribus, Trilinos,...


--
G. Jungman

Re: Feedback from GSL folks on libflame 4.0

[Field G. Van Zee]

Gerard Jungman wrote:
> 
>> 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.

Perhaps a dumb question: Why are the global scalars not a problem? Is it because
they are only read by the user and never updated?

The memory leak counter is not crucial. It's a nice way to quickly tell if you
are freeing all your objects, but that's it. I think we could live without it.
Or, as you said, we could go the errno route. We would need someone else (Brian)
to explain how exactly to implement this sort of thing.

There are some other global variables that are set via FLA_Init() that have to
do with our control tree infrastructure. I'm not sure if we can get by without
them. There is quite a bit of cleanup to do on this front anyway. Perhaps I
should do that first before trying to achieve further thread safety.

> In the final picture, I think thread-safety is really important.
> More and more, it's going to become a requirement for new code.

Agreed.

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

In very early versions of libflame (before it was even called "libflame") there
was only one object struct which contained all fields. Later, Robert separated
the "view" data from the base object data. It seemed like the smart thing to do
at the time, but I can see how it might incur quite a bit more memory overhead.
I can see arguments going both ways on whether we need to worry about that extra
memory cost.

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

This seems like a reasonable solution. I'll look into whether it is workable in
libflame. (We have a LOT of functions that would need to be changed.)

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

We would love to help you eliminate your CBLAS code. Check back with us this
summer to see how we're progressing on this front.

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

All messages should be printed via FLA_Print_message(). Should. I'd need to do a
careful audit to make sure this is the case. So, C standard I/O should be
confined to that function. But let me look into this.

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

We recently added shared library generation. We compile the source with -fPIC
and then link with -shared. Worked the first first time I tried it, and so it
seemed too good/easy to be true. Am I missing something?

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

You seemed pretty well-informed for just an afternoon of code/doc browsing! We
will try to stay tuned in to discussions that happen here on gsl-discuss, and
look forward to working with you guys. Thanks again!

Field

Re: Feedback from GSL folks on libflame 4.0

[Field G. Van Zee]

Brian,

Thanks for the suggestion. I'm preparing to leave for SIAM 2010, but once I get
back I will look into implementing some changes to the error-checking in
libflame. We may have to go Gerard's route and have dual interfaces, or we might
do it as you propose. Error checking right now in libflame is a bit of an
organized hack, and so I'll need to sit down and look at just how much code will
need to be updated to implement any changes.

As always, we appreciate your input!

Field

Brian Gough wrote:
>> 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.
> 
> The approach in GSL is to have an abort() by default, for the same
> reason.  But the user can turn off the abort() and use the error
> return values instead by providing an alternative error handler as a
> function pointer.  Here's how it would look with your functions:
> 
> === FLA_macro_defs.h ===
> 
> // Error code translation and output macro definition.
> #define FLA_Check_error_code( code ) \
>   do { \
>        if (code != FLA_SUCCESS) {  \
>          FLA_Check_error_code_helper( code, __FILE__, __LINE__ ); \
>          return code; \    /* see comment below about void functions */
>        } \
>   } while (0)
> 
> 
> === FLA_main_prototypes.h ===
> 
> typedef void FLA_error_handler_t (int code, const char * file, int line);
> 
> 
> === FLA_Check.c ===
> 
> static FLA_error_handler_t * FLA_error_handler = NULL;
> 
> void FLA_Check_error_code_helper( int code, char* file, int line )
> {
>   if ( code == FLA_SUCCESS )
>     return;
> 
>   if (FLA_error_handler) 
>     {
>       (*FLA_error_handler) (code, file, line);
>       return ;
>     }
> 
>   if ( FLA_ERROR_CODE_MAX <= code && code <= FLA_ERROR_CODE_MIN )
>   {
>     FLA_Print_message( FLA_Error_string_for_code( code ),
>                        file, line );
>     FLA_Abort();
>   }
>   else
>   {
>     FLA_Print_message( FLA_Error_string_for_code( FLA_UNDEFINED_ERROR_CODE ),
>                        file, line );
>     FLA_Abort();
>   }
> }
> 
> 
> (We actually have three macro GSL_ERROR, GSL_ERROR_VOID and
> GSL_ERROR_VAL to handle different types of return value - error code,
> void, and "other".  This is to avoid warnings about "returning a value
> from a void function" etc).

Re: Feedback from GSL folks on libflame 4.0

[Brian Gough]

At Fri, 19 Feb 2010 13:58:06 -0700,
Gerard Jungman wrote:
> 
> On Fri, 2010-02-19 at 13:41 -0500, Robert G. Brown wrote:
> > On Fri, 19 Feb 2010, Brian Gough wrote:
> > >
> > > Thanks for using autoconf. I think it is a good thing.
> > 
> > Yeah, I don't really think of them as optional these days.  They are far
> > from perfect and often a PITA, but what they do is necessary.
> 
> You guys are missing the point. The question is not
> "autotools or nothing" but "autotools or cmake".

As Robert says, it's a question of standardisation.  I have nothing
against cmake, on a purely techical level it may be better but
autotools is the defacto standard and that is worth more in practice.

Re: Feedback from GSL folks on libflame 4.0

[Theodore Papadopoulo]

On 02/22/2010 11:27 PM, Brian Gough wrote:
> At Fri, 19 Feb 2010 13:58:06 -0700,
> Gerard Jungman wrote:
>    
>> You guys are missing the point. The question is not
>> "autotools or nothing" but "autotools or cmake".
>>      
> As Robert says, it's a question of standardisation.  I have nothing
> against cmake, on a purely techical level it may be better but
> autotools is the defacto standard and that is worth more in practice.
>
>    
Well, just to add my 2cents. The world is changing and cmake is becoming 
very fast
the standard. Many projects have switched to it. One of the major 
advantage is that
it works under windows out of the box...

I'm saying that all the more that I used to be an autotools convinced 
guy (and not very statisfied
by cmake some time ago), but cmake is progressing rapidly. As said above 
quite a few
opensource projects adopted it and it is taken into account in many 
distro build system.

I can't believe I'm saying that, but it's true...

     All the best,

         Theo.

Re: Feedback from GSL folks on libflame 4.0

[Brian Gough]

At Fri, 19 Feb 2010 22:30:36 -0600,
Field G. Van Zee wrote:
> Or, as you said, we could go the errno route. We would need someone
> else (Brian) to explain how exactly to implement this sort of thing.

Using errno in a multithreaded program relies on operating system
support for thread-local storage, which creates a new set of
portability problems of its own.

> We recently added shared library generation. We compile the source
> with -fPIC and then link with -shared. Worked the first first time I
> tried it, and so it seemed too good/easy to be true. Am I missing
> something?

It is that simple on GNU/Linux, but other operating systems have
different incompatible methods.  Also, there is the issue of
supporting shared library versions if you want to allow the library to
be safely updated independent of any applications using it.

-- 
Brian Gough

Re: Feedback from GSL folks on libflame 4.0

[Robert G. Brown]

On Tue, 23 Feb 2010, Theodore Papadopoulo wrote:

> I'm saying that all the more that I used to be an autotools convinced guy 
> (and not very statisfied
> by cmake some time ago), but cmake is progressing rapidly. As said above 
> quite a few
> opensource projects adopted it and it is taken into account in many distro 
> build system.
>
> I can't believe I'm saying that, but it's true...

Well, when I next have time to mess with it I'll see if I can port my
own major projects to it -- again.  Last time it was a disaster.

I don't suppose they provide an "autotools2cmake" utility, do they?

    rgb

>
>    All the best,
>
>        Theo.
>

Robert G. Brown	                       http://www.phy.duke.edu/~rgb/
Duke University Dept. of Physics, Box 90305
Durham, N.C. 27708-0305
Phone: 1-919-660-2567  Fax: 919-660-2525     email:rgb@phy.duke.edu

Re: Feedback from GSL folks on libflame 4.0

[James Amundson]

On 02/22/2010 04:27 PM, Brian Gough wrote:
> As Robert says, it's a question of standardisation.  I have nothing
> against cmake, on a purely techical level it may be better but
> autotools is the defacto standard and that is worth more in practice.
>    

I'm entirely sympathetic with the argument that existing standards are 
very valuable and may trump technical advantages in the end. I don't 
think the case for autotools being "standard" and cmake being 
"non-standard" are so clear, however. Let's look at the other 
mathematical libraries I deal with on a regular basis:

1) FFTW
2) LAPACK
3) PETSc ( http://www.mcs.anl.gov/petsc/ )
4) Triinos ( http://trilinos.sandia.gov/ )

What do they use for build systems?

1) autotools
2) Custom makefiles
3) Custom complex build system
4) cmake

No clear winner there. Let's look at some major components of my linux 
installation:

1) Linux kernel
2) GNU utilities
3) Gnome
4) KDE

What doe they use for build systems?

1) Custom makefiles
2) autotools
3) autotools
4) cmake

Of course, this isn't a detailed survey. I assume autotools would have 
the numerical advantage over all other options if a detailed analysis 
was made. The point is, however, that many projects don't use autotools 
and that a signficant number use cmake. I don't think the "standard" 
argument is strong enough to simply trump any technical argument. Of 
course, the technical argument still needs to be made.

If you were to argue that the current level of predominance of autotools 
means that autotools is the only acceptable build system for GSL, I 
think I could make a similar argument that the current level of 
predominance of Fortran in scientific computing means that Fortran is 
the only acceptable  implementation language for GSL.

--Jim Amundson

Re: Feedback from GSL folks on libflame 4.0

[Gerard Jungman]

On Fri, 2010-02-19 at 22:30 -0600, Field G. Van Zee wrote:
> 
> In very early versions of libflame (before it was even called "libflame") there
> was only one object struct which contained all fields. Later, Robert separated
> the "view" data from the base object data. It seemed like the smart thing to do
> at the time, but I can see how it might incur quite a bit more memory overhead.
> I can see arguments going both ways on whether we need to worry about that extra
> memory cost.

Memory overhead is not the issue. The problem is that any use of
the C heap (malloc()/free() and friends) ties the implementation
to that one allocation strategy. This makes it hard (impossible ?)
for other environments to control the allocation strategies.
Compare to the C++ STL containers, where the allocation strategy
is parametrized (literally a template parameter), by design.

Separating the views and the base objects is a good idea, since
the views are (at least in principle) light-weight objects. But
tying the composition of these objects to a heap allocation is
restrictive.

The way I think of this is that the 'view' is like the base class
in the hierarchy. What you call the 'base object' is actually
the derived object, since both views and bases must export the
interface for access, but the object with the data extends
this functionality by actually managing the data segment.

Sorry if I'm stating obvious stuff. I'm sure you have thought
about these things. But I wanted to explain how I look at it.
I don't know if there is an easy answer for this type of
container design in C, but its good to know the trade-offs.

--
G. Jungman

Re: Feedback from GSL folks on libflame 4.0

[Gerard Jungman]

On Tue, 2010-02-23 at 09:54 -0500, Robert G. Brown wrote:
> 
> I don't suppose they provide an "autotools2cmake" utility, do they?

KDE used something of this type, I think its called "am2cmake";
you can google it up.

But, as somebody told me recently, it's
hard to turn the soup back into vegetables.

--
G. Jungman

Re: Feedback from GSL folks on libflame 4.0

[Robert G. Brown]

On Tue, 23 Feb 2010, Gerard Jungman wrote:

> KDE used something of this type, I think its called "am2cmake";
> you can google it up.

Thanks, I'll give it a try on my next project or when I have time to
monkey with a fork of my current ones.

> But, as somebody told me recently, it's
> hard to turn the soup back into vegetables.

Well, one would HOPE that the information entropy of a software tree was
a lot less than that of soup -- maybe even something like "zero" -- but
yeah, the problem is difficult all around.  Life would be so simple if
all of the other operating systems and distributions and hardware
environments but the one >>I<< work on would have the good grace to just
up and die...;-)

And it really is a lovely adage.  I'll have to keep that one in mind:-)

    rgb

Robert G. Brown	                       http://www.phy.duke.edu/~rgb/
Duke University Dept. of Physics, Box 90305
Durham, N.C. 27708-0305
Phone: 1-919-660-2567  Fax: 919-660-2525     email:rgb@phy.duke.edu