xserver: Cleaning up memory allocation functions and macros

[Daniel Stone]

On Sun, Apr 29, 2007 at 08:18:33PM +0200, Egbert Eich wrote:
> Magnus Vigerlöf writes:
>  > * I haven't seen any place where INTERNAL_MALLOC is set/used (except in 
>  > os/xalloc.c and os/utils.c) so let's remove every trace of that one. This 
>  > also means that os/xalloc.c will be removed.
> 
> The internal malloc was introduced when Linux was using glibc and it's
> own libc malloc sucked. Today with the glibc malloc we don't have these
> problems any more.

I really wish we'd stop trying to solve other peoples' problems.  It
wasn't a particularly good idea when we tried to fix malloc; it wasn't a
particularly good idea when the XFree86 DDX tried to route around DIX
damage (e.g. the input disaster); it wasn't a good idea when drivers
tried to route around server damage (e.g. MergedFB).  You can find many,
many examples, and almost all of them have been really bad.

> It contained a rudimentary memory debugger to catch double free's for 
> example which I was using from time to time as it was easy and straight
> forward to use.

glibc already does this.  If you're really bored, you can use one of
several gcc patchsets (ProPolice, SSP, et al) to catch stack smashes as
well.  I'm sure the other platforms have similar functionality.

(And, if not, it's not our problem: see above.)

> Again a document on resources for memory debugging using the system
> tools would be useful.

If you grab git://people.freedesktop.org/~daniels/valgrind, you can just
valgrind the server.

> Are you suggesting to call the system provided malloc() from the code
> directly?

Is there a reason not to?  It's portable and generally highly optimised.

> I'm not sure if this is a great idea: A lot of bigger projects create 
> their own allocation calls mostly probably for portability reasons:
> as was the case with early Linux sytems one may want to port the code
> to a platform whose internal malloc is not really optimal for our purposes.

Fix your platform.

Any malloc() we write will be, by definition, bad.  It might work okay
for a while.  But it will always be suboptimal compared to that of any
system-provided platform[0].

We write window systems, not C libraries or build systems.  If your
platform has a broken malloc() -- fix it.  The X server _and everything
else_ will get a whole heap faster.  I do not see why this is our
problem.

> With own wrappers or just macros it's easy to remedy this situation
> without uglu kludges.

The entire remedy you're proposing _is_ an ugly kludge.

> X.Org has always also been the sample implementation of the X Window
> System. Therefore the goal has been to make it portable enough that
> it can be ported to whatever OS there is (*).
> Have we abandoned this goal? - To ensure the maximum portability 
> either macros or wrappers seem to be advisable.

malloc() is portable.  Attempting to write our own is suicide.  What
you're proposing is that we write our own C library, because some C
libraries out there somewhere aren't very good.  Even leaving aside the
fact that we don't even have enough developer resources currently to
just deal with the server, libraries, and clients, we are never going to
match glibc or Sun's libc.

When we tried with the loader ('not all platforms have libdl, so let's
write our own'), we ended up exacerbating the problem.  To help LynxOS
or Amoeba or something, meant that we were unable to function on
Linux/IA64, Linux/HPPA, Linux/SH, and a great many other ports, because,
surprisingly, no-one wanted to write an entire ELF loader for their
particular platform, inside the X server.

The use of malloc() is _not_ a portability problem.  If your platform
has a slow malloc(), then that is exactly the problem: _your platform_
has a slow malloc().

If the X server exhibits particular pathological behaviours, then we can
and should fix it.  But if your platform has particular endemic
problems, then I can tell you exactly where the fault and responsibility
lies.

I don't think you'll find anyone here opposed to actual portability.
We've always been quite careful to restrict the subset of the C language
and particular functions we use, and generally ask to make sure that any
new usage won't break various ports.

>  >   - free() can take NULL as argument, so it's not needed to test for NULL 
>  > before calling it.
> 
> Maybe not on all implementations? If we use a wrapper or macro this
> can be handled there. The default wrapper doesn't need to address this.

Check what POSIX/SuS says.  If it can take it, then we're fine.

> But they may be handy in situations where a failure is unlikely (so we 
> don't want to be bothered with special handling) but if it happens need 
> to be handled with a proper error message.
> Driver initialization is a situation that comes to my mind here.

Using xnf* _is_ special handling.

> (*) The notion of sample implementation doesn't mean that we have or 
> want to support all these OSes. The idea is that anybody can pick up
> the code and port it to his operating system. This notion has helped
> to proliferate the X Window System.
> And no, I probably don't want to see support of all these OSes clutter
> up our tree. 

Again, malloc() does not present a portability risk.  As long as we
don't do anything that actively harms portability, then we're okay.  If
particular platforms have their own problems, than that is exactly that:
_their own_ problem, not ours.  Let's try to fix our own issues in X
before we start trying to fix peoples' memory allocators.

Cheers,
Daniel

[0]: Or, in the case of open-coded functions like strcasecmp(),
     demonstratably _worse_ than any implementation that wasn't actively
     attempting to be slow.  Aside from the stack smash and the
     incorrect return codes, xkbcomp execution time quite literally
     halved on the N800 when we switched to the libc-provided one.
     Also, there are often numerous platform-specific tricks you can
     provide to make these functions blindingly fast: cf. memcpy on
     glibc/ARM.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg/attachments/20070429/a17392a2/attachment.pgp>