This is the mail archive of the gsl-discuss@sourceware.org mailing list for the GSL project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Feedback from GSL folks on libflame 4.0


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]