Re: [PATCH] Slightly better way to __USE_MALLOC

[Brad Spencer <spencer at infointeractive dot com>]

On Thu, Oct 17, 2002 at 11:16:19AM -0500, Loren James Rittle wrote:
> Hi Brad (and members of the list that care),
> 
> Sorry about the long delay in getting back to you.  Your work (BTW,
> consider this a response to the actual patch you sent last Thursday as
> well) is very helpful even with my comments below.  Actually, if you
> are willing, I'd like you to think more radically (to that end, I have
> attached such a patch to this e-mail, please tell me if you would
> dislike my proposed patch verses your approach).

I think this is an excellent patch!  I've been testing a couple of
slight variations and seeing how it performs against the gcc-3.2
release.  I agree with your criticism of my previous patches.

> Instead of that perspective, let's see how to fix this right.  (BTW,
> as you know, the exact problem with the header is an undetectable
> violation of the one-definition-rule and your current patch just adds
> one extra level of checking about the situation but the root violation
> would still be present.)  Then, we test a correctly partitioned
> version against the current one and see if it matters for real
> application code (the code in testsuite/thread/pthread[1-6].cc
> stresses the memory allocators; or may be configured to do so, if you
> don't believe that they do for your machine).

Indeed, I work in an exclusively single-threaded universe, but clearly
not everybody does.  "Optimal" performance for everybody would be
best in my mind, too.

> I would like to see the __USE_MALLOC macro-enabled path completely
> gone not renamed.  Although users could get it, that path was never

This line of reasoning sounds good to me, BTW.

> I hope this late feedback and alternate patch is helpful without
> making you in any way dejected.

Not at all!  You and others have been encouraging and supportive.  I
didn't expect to rewrite stl_alloc.h properly on my first try ;)

> Attached is the patch I would want to apply to mainline to address
> this issue.  My patch shows 4 additions to the ABI: the new statics
> and their guards.  However, I don't believe that they would need to be
> exposed for correct operation.  Benjamin, Phil, little help on that
> detail (it seems we currently unconditionally globalize all such
> statics and their guards)?  Of course, I'm willing to negotiate on any
> part of what I did here, e.g. I could add back in the typedef alias
> for __mem_interface since it was in the header ABI (but not the binary
> library) and perhaps for small embedded targets we really do need to
> merge in something like Brad's configuration-time style as well (but I
> have the reservations listed above).

Presumably this would require the symbol mismatching I included.
Would these extra statics per object themselves be problematic for
embedded targets?

> Rebuilt libstdc++-v3 from scratch on alpha-unknown-freebsd4.7 and
> i386-unknown-freebsd4.7.  No regressions.  Also ran `gmake check' with
> GLIBCPP_FORCE_NEW=1 in environment.

I will run this myself when I can.  I wrote a simple loop that
allocated and freed and timed that just to get a measure of the
performance of the allocation decision logic.

#include <memory>

int
main()
{
  std::allocator<char> alloc;
  
  for(unsigned int i = 0; i < 10000000; ++i) {
    char *memory = alloc.allocate(64);
    alloc.deallocate(memory, 64);
  }

  return 0;
}

Here are some timings when compiling with -O6 and running with and
without GLIBCPP_FORCE_NEW.  Time quoted are user time from "time
./main". 

                       Normal       With GLIBCPP_FORCE_NEW=1
gcc-3.2             :  0m0.250s     n/a
cvs gcc             :  0m0.260s     n/a
cvs gcc w/your patch:  0m0.320s     0m1.470s
cvs gcc w/your patch
  using class static:  0m0.270s     0m1.730s
  [see my first comment below]

> Comments?

By checking the environment variable in both allocate() and
deallocate() (for any given instance of the class template), are we
setting up for someone to change it in between the first calls to
those methods?  Now, granted, that would be evil, but perhaps making
it a class static constant is better?  Given the hint that this is a
performance boost, too, perhaps it's worthwhile.  I've attached a
cumulative patch for stl_alloc.h that includes your changes but makes
this change.  

I don't understand why the class-static version so greatly affects the
GLIBCPP_FORCE_NEW time.  Perhaps the test routine is simply too naive.

For the application to select the allocation scheme explicitly at
runtime, it would have to ensure it called putenv() before any use of
std::allocator.  Is this possible?  I'm not certain it's a desirable
feature, but perhaps someone would want to release a certain
application that performs better one way or the other and they want to
hardwire it that way.  This wouldn't be a property of the target as
much as it is of the application itself?

Regardless, this is going to be a joy to use! :)

-- 
------------------------------------------------------------------
Brad Spencer - spencer@infointeractive.com - "It's quite nice..."
Systems Architect | InfoInterActive Corp. | A Canadian AOL Company

Attachment: loren.class-static.patch
Description: Text document