apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Reid" <dr...@jetnet.co.uk>
Subject Re: Memory Renaming (try 2)
Date Sat, 12 May 2001 13:37:35 GMT
You know it's not a race guys...

The reason that no-one has changed the code yet is that we haven't really
had enough time to talk about it.  The changes will get made, but it
requires patience at the moment.

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

This will be addressed...

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

This will need looking at.  Lets get some working code that we all agree is
the way forward before we start getting nitpicky shall we?

> > 3) There is some logic that you are attempting to duplicate in
> >    apr_sms_realloc() that I believe the realloc system call handles for
> you
> >    (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
> is minor overhead and lessens the work for a sms implementor. For
> performance
> I have to agree, you move the 'cleverness' to the specific sms
> implementation,
> or you don't implement it at all. IMHO the only sms that doesn't need the
> 'cleverness' is the standard sms.

Same as above.

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

Consider it yanked.

> > 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
> >    odd wrapping properties.  This isn't my code, so it's not a big deal
> >    me. Longer variables names are *slightly* better than completely
> cryptic
> >    variable names...
> Like I stated before, the code was also the documentation in our
> (among Elrond, Luke and me). I have no problem with shorter variable
> as long as the code stays readable. I tend to believe that for at least
> of the (to be) shortened variable names a comment is introduced.

And if it's me that goes through and does the renaming this will be taken
care of.

> > 5) What do you plan apr_sms_threadsafe_lock to do (maybe you talked
> >    this, but I missed it)?  Is this something similar to apr_lock? Why
> >    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
> you.
> The name is confusing as I stated in earlier postings.

There has been much discussion of this already.

> > 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
> around
> >    line 400.)
> I posted a 'typed cleanup function' patch that includes fixes for this (as
> side-effect of better status reporting).

Well, there are lots of cleanups still required in the code.  It's taken a
fair bit of time to get it into the codebase as it stands now, and more
effort will be required.  When I get a better connection I'll be doing more.

> > 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
>  have a memory_common.c in the win32 directory that simply includes the
>  in the unix directory."

Hopefully that answers it.

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

There are a lot of people with "karma" but as I said the way we do things is
to discuss then commit.  AFAIK that's how things have always been done round
here, so please try to be patient.  It will happen.

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

I would have thought this was an obvious step, but maybe not.  Hopefully the
explanation helped.

> 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
> memory
> allocation availabe and it should be possible to use that (but this is
> my opinion).

Well we'll have to see where this all goes won't we?


View raw message