apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sander Striker" <stri...@samba-tng.org>
Subject RE: Memory Renaming (try 2)
Date Sat, 12 May 2001 11:41:37 GMT
> Since the build doesn't generate the makefiles for memory (and am
> too tired to dig around myself and get it to do it right now), I can't
> tell you whether the original code or my renamed code compiles.

Just add 'memory' to 'apr_modules' in configure.in and rerun
buildconf, configure and make, it should work then (Thanks David).

> My comments:
> 1) Said before, but asserts are yucky.  Once we have it compiling and
>    working, this will be critical to remove.

I guess this will stay a difference in opinion, even within the APR team.
But, since this code is (going to be) part of the APR library, it is now
your call. Personally I would have my code rather die at debug time so
I am confronted with my stupidity (and as a side-effect have faster running
release code, because the asserts are defined to noop) than have the code
silently continue and somewhere else fail. In the latter case I even
have a harder time finding where the problem is.

In one way or another you are forcing users of the APR to use a certain
coding style:
 - with asserts you force them to pass valid parameters (at debug time)
 - without them you force them to check return values

Just my $0.02, do with it what you like, I just wanted to have it said.

> 2) From the performance standpoint, we need to make this malloc stuff as
>    fast as the plain malloc call or this isn't going to be worth much.
>    Checks such as "if !size return 0" probably isn't needed in
>    apr_sms_malloc() code.

Probably not. What does malloc() return when passed a size of 0?
Btw, sms _always_ adds some overhead, but what you get back for it is
(a bit of) flexibility.

> 3) There is some logic that you are attempting to duplicate in
>    apr_sms_realloc() that I believe the realloc system call handles for
>    (at least on Solaris).  More generally, you are *probably* better off
>    letting the system calls handle this type of redirection rather than
>    yourself, but I might be wrong.  Dean may have a better idea
>    whether we should try to outsmart the system calls or not - my gut
>    feeling would be to avoid this "cleverness."

The same goes for realloc() as goes for malloc(), however, this 'cleverness'
is minor overhead and lessens the work for a sms implementor. For
I have to agree, you move the 'cleverness' to the specific sms
or you don't implement it at all. IMHO the only sms that doesn't need the
'cleverness' is the standard sms.

>    (You also try to be "safe" about a memset call - if memset is bogus,
>    we have lots of other problems...)

Ok, so we ditch the memset and set all members of apr_sms_t to NULL by hand.

> 4) The function and type rename is a good step, but I'd probably like to
>    see shorter local variable names.  Makes for some long lines that have
>    odd wrapping properties.  This isn't my code, so it's not a big deal to
>    me. Longer variables names are *slightly* better than completely
>    variable names...

Like I stated before, the code was also the documentation in our discussion
(among Elrond, Luke and me). I have no problem with shorter variable names,
as long as the code stays readable. I tend to believe that for at least 25%
of the (to be) shortened variable names a comment is introduced.

> 5) What do you plan apr_sms_threadsafe_lock to do (maybe you talked about
>    this, but I missed it)?  Is this something similar to apr_lock? Why are
>    we concerned about an apr_sms_t being shared across threads?  (Please
>    don't tell me shared memory segments...)

*grin* It was meant as the start of support for the thing I shouldn't tell
The name is confusing as I stated in earlier postings.

> 6) Somewhere there is a bunch of if, else if statements that are
>    separated by a bunch of blank lines.  Yuck.  (apr_memory_system.c
>    line 400.)

I posted a 'typed cleanup function' patch that includes fixes for this (as a
side-effect of better status reporting).

> 7) Why is this stuff in memory/unix?  Wouldn't most of this work
>    on Win32, etc.?  ANSI C requires most of the stuff you are using in
>    apr_sms_std (malloc, free, etc.).  But, I could be missing something.

I believe David Reid already answered this one. I quote:

"Try looking at the rest of the code in APR :)  Win32 can build in a Unix
 directory, but eventualy there will be some custom Win32 code and so we'll
 have a memory_common.c in the win32 directory that simply includes the code
 in the unix directory."

> 8) Someone with karma should probably rename the files to match the
>    appropriate names (apr_sms.h, etc.).

Ack. Could someone with the karma please rise :-)

> 9) Why is apr_tracking_memory_system.h separated out?  This patch
>    pulls it back in to (what should be) apr_sms.h.  I can't figure out
>    any logical reason why apr_tracking_memory_system.h exists.

Because 'tracking sms' and 'standard sms' are just two implementations.
There are a lot more to come (I hope). Since you are always going to
need the standard sms it is included in (what should be) apr_sms.h.
The 'tracking sms' (only provided as an example btw) is just on of the
possible implementations of a custom tracking sms. It is therefor a
seperate module. Speaking of modules, someone already suggested dynamicly
loading sms modules :-), which could be another good reason.

To make another thing clear, I think it should be allowed to do platform
specific memory systems. On some platforms there is probably very fast
allocation availabe and it should be possible to use that (but this is just
my opinion).

> My $.02.  It's 3:30AM in the morning here, so if this post doesn't make
> much sense, that's probably why.  -- justin

Ah, I know how that is :-)


View raw message