You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@bellsouth.net> on 2000/10/13 16:10:33 UTC
Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
rbb@locus.apache.org writes:
> rbb 00/10/12 22:35:58
>
> Modified: src CHANGES
> src/ap ap_buckets.c ap_buckets_eos.c ap_buckets_file.c
> ap_buckets_heap.c ap_buckets_mmap.c
> ap_buckets_pipe.c ap_buckets_simple.c
> ap_buckets_socket.c
> src/include ap_buckets.h
> src/main http_core.c http_protocol.c util_filter.c
> src/modules/experimental mod_charset_lite.c
> src/modules/standard mod_include.c
> Log:
> Remove all function pointers from the ap_bucket type. These function
> pointers are replaced with a global table that allows modules to register
> their bucket types. Those bucket types are then allowed to be used in
> the server processing. This also required removing all direct calls to
> those functions. The ap_bucket type has an index into an array, so in
> each ap_bucket_* function, we use that index to find the correct set of
> functions.
What is the purpose of this? We just got even slower.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by Jeff Trawick <tr...@bellsouth.net>.
rbb@covalent.net writes:
> On Fri, 13 Oct 2000 rbb@covalent.net wrote:
>
> > > I'm completely with Jeff on this one. I see no gains, and only negatives, in
> > > this "improvement". Shrinking the bucket size buys us very little, and (as
> > > Jeff says) the enumerated type is only important for us to write code to
> > > detect a few, special types. Obviously, we couldn't have code to look for a
> > > third-party type, so it doesn't matter what their bucket type is.
> > >
> > > In fact, I'd formalize it and introduce AP_BUCKET_TYPE_EXTENSION. All
> > > extensions to the bucket system can use that.
>
> BTW, that won't work. Imagine a bucket that mod_perl creates and wants to
> use to do some special processing with. Using the same type as is used
> for an SSL socket bucket would break that completely.
>
> Ryan
That is where ap_get-me-a-bucket-type() comes in. A module
implementing a custom bucket type can then get a unique id which
doesn't conflict with the ids of core bucket types.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by rb...@covalent.net.
On Fri, 13 Oct 2000 rbb@covalent.net wrote:
> > I'm completely with Jeff on this one. I see no gains, and only negatives, in
> > this "improvement". Shrinking the bucket size buys us very little, and (as
> > Jeff says) the enumerated type is only important for us to write code to
> > detect a few, special types. Obviously, we couldn't have code to look for a
> > third-party type, so it doesn't matter what their bucket type is.
> >
> > In fact, I'd formalize it and introduce AP_BUCKET_TYPE_EXTENSION. All
> > extensions to the bucket system can use that.
BTW, that won't work. Imagine a bucket that mod_perl creates and wants to
use to do some special processing with. Using the same type as is used
for an SSL socket bucket would break that completely.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by rb...@covalent.net.
> > No, I'm in no hurry to go any further down this path. I don't think
> > what is there already is worthwhile.
> >
> > We don't have better performance and we don't need the change in order
> > to allow modules to implement their own bucket types.
>
> I'm completely with Jeff on this one. I see no gains, and only negatives, in
> this "improvement". Shrinking the bucket size buys us very little, and (as
> Jeff says) the enumerated type is only important for us to write code to
> detect a few, special types. Obviously, we couldn't have code to look for a
> third-party type, so it doesn't matter what their bucket type is.
>
> In fact, I'd formalize it and introduce AP_BUCKET_TYPE_EXTENSION. All
> extensions to the bucket system can use that.
>
> Ryan: will you back that change out?
There are a couple of major improvements that haven't been discussed yet
that this does buy us.
#1) The ability to let the bucket type grow and mature without requiring
us to re-compile the whole server. I know binary compatability isn't a
big issue, but it is there.
#2) We go from making five assignments when creating buckets to making
one. This is a performance improvement when creating buckets.
#3) This mimics FreeBSD's kobj code, the guy who did that work did a
thorough performance analysis, and found only a negligable performance hit
came with the redirection. Granted, we will see a bigger hit until we
move to a pointer/macro based system, but that would only take a few hours
to implement.
The arguments against this so far have been performance, which I
personally don't agree with, and the fact that we can't tell what type we
are, which is solvable in this framework. This is far more flexible, and
it keeps the bucket size smaller, in fact we shrunk it by about half. Not
to mention that this improvement was mentioned in the big filter meeting,
where everybody generally agreed that getting the function pointers out of
the bucket type was a good idea.
I would prefer to hear from some of the other people before I back this
out. I know Tony was planning on doing this at some point, but he is
currently in the middle of moving I believe.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Oct 13, 2000 at 03:25:27PM -0400, Jeff Trawick wrote:
> rbb@covalent.net writes:
>
> > I had planned on letting this code work for a few days before making these
> > changes, because I wanted to exercise the code before I macro-ized it, but
> > I'm flexible.
>
> No, I'm in no hurry to go any further down this path. I don't think
> what is there already is worthwhile.
>
> We don't have better performance and we don't need the change in order
> to allow modules to implement their own bucket types.
I'm completely with Jeff on this one. I see no gains, and only negatives, in
this "improvement". Shrinking the bucket size buys us very little, and (as
Jeff says) the enumerated type is only important for us to write code to
detect a few, special types. Obviously, we couldn't have code to look for a
third-party type, so it doesn't matter what their bucket type is.
In fact, I'd formalize it and introduce AP_BUCKET_TYPE_EXTENSION. All
extensions to the bucket system can use that.
Ryan: will you back that change out?
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
RE: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
> From: Jeff Trawick [mailto:trawickj@bellsouth.net]
> Sent: Saturday, October 14, 2000 12:48 AM
>
> > I like the thought of being able to extend -what- filters
> can do, and
> > -how- they can do them. Extending into the vast unknown
> future bucket
> > types seems useful in ways we can't predict yet. I'm just
> making certain
> > that your performance issue is really the issue, or did we
> jam things up
> > somewhere else?
>
> Nobody is speaking against such an extension; I'm definitely for it.
> What I'm speaking about is more code in the path which we don't need.
Then that would be solved with the macros Ryan is offering up - I don't
disagree though that he wants to see the code work first.
here's what I think Ryan and I worked out...
the table is soley for reserving a numerical offset
the bucket itself will have a pointer straight to the bucket fn list
structure. The table lets us walk valid bucket types later on, but
not on a regular basis.
the fn ptr structure will include a char* bucketname
it won't be used often, but it will help in debugging and will be
the helper for a mod_dav_fs type module that -must- have an intimate
knowledge of bucket type installed by a mod_dav type parent module.
It can query by name against the table, but this would be done
once and cached for the life of the server.
the fn ptr structure will include a 'feature count' - number of
supported Apache bucket functions. We haven't worked that whole
thought out (who can define new fns - Apache core - mod authors?).
the fn ptr structures are 8 statics for the 8 core types, and they are
always in the enumeration's order.
all fn ptrs are initialized - worst is fn returning APR_NOTIMPL.
We have to test that the read/setaside/whatever actually succeeds,
so we drop a null ptr test for an extra error to be resolved
(the exception case instead of the rule).
Jeff - if a new bucket type queries apr -only once- for it's structure,
and gets it's assignment for the entire life of the server, and buckets
know their bucket type by a pointer to their fn ptrs structure, and you
can query the type of the bucket by name --- will that solve your issues?
RE: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
> From: Jeff Trawick [mailto:trawickj@bellsouth.net]
> Sent: Saturday, October 14, 2000 12:48 AM
> To: new-httpd@apache.org
> Subject: Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
>
> > I like the thought of being able to extend -what- filters can do, and
> > -how- they can do them. Extending into the vast unknown future bucket
> > types seems useful in ways we can't predict yet. I'm just making certain
> > that your performance issue is really the issue, or did we jam things up
> > somewhere else?
>
> Nobody is speaking against such an extension; I'm definitely for it.
> What I'm speaking about is more code in the path which we don't need.
Then I need to take a close look at the code we don't expect we need... can
I read the tree or did you last proposed patch overlay the behavior you
take issue with?
I've been a c++ com programmer for years - and I know many folks look at it
as wasteful - but if you are tight (we are) you shouldn't be wasting more
that a few cpu clicks with the double indirections. Now that I'm understanding
here what Ryan proposes - give me a little time to a close look at it before
we talk about backing it out ... I'm a little frustrated with all the talk in
the alpha of 'backing out' - we should be pushing forward and improving.
Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by Jeff Trawick <tr...@bellsouth.net>.
"William A. Rowe, Jr." <wr...@rowe-clan.net> writes:
> We've indirected the bucket object's vtble into it's own object. What's
> the issue here? Based on the cpu's cache and the frequency we hit these
> vtbls (see Roy's latest lecture), they should already land in cache.
> I guess I want to know what performace hit you are seeing, it can't
> simply be the indirection.
In this case, in-the-cache means not as much extra overhead as there
could be.
> I like the thought of being able to extend -what- filters can do, and
> -how- they can do them. Extending into the vast unknown future bucket
> types seems useful in ways we can't predict yet. I'm just making certain
> that your performance issue is really the issue, or did we jam things up
> somewhere else?
Nobody is speaking against such an extension; I'm definitely for it.
What I'm speaking about is more code in the path which we don't need.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
RE: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
> From: Jeff Trawick [mailto:trawickj@bellsouth.net]
> Sent: Friday, October 13, 2000 2:25 PM
>
> rbb@covalent.net writes:
>
> > I had planned on letting this code work for a few days
> before making these
> > changes, because I wanted to exercise the code before I
> macro-ized it, but
> > I'm flexible.
>
> No, I'm in no hurry to go any further down this path. I don't think
> what is there already is worthwhile.
>
> We don't have better performance and we don't need the change in order
> to allow modules to implement their own bucket types.
Ok... I'm confused... time to play C++ programmer here...
We've indirected the bucket object's vtble into it's own object. What's
the issue here? Based on the cpu's cache and the frequency we hit these
vtbls (see Roy's latest lecture), they should already land in cache.
I guess I want to know what performace hit you are seeing, it can't
simply be the indirection.
I know I go overboard in trying to make things generic myself, but if
we follow this design in the direction we are heading, the filter
knows nothing of it's upstream and downstream neighbors, only what the
-current- request and requested results should be. So you know the
data -was- in utf-8, you know the client -wants- cp-850, and you happen
to be able to do that since you are a charset filter.
I like the thought of being able to extend -what- filters can do, and
-how- they can do them. Extending into the vast unknown future bucket
types seems useful in ways we can't predict yet. I'm just making certain
that your performance issue is really the issue, or did we jam things up
somewhere else?
Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by Jeff Trawick <tr...@bellsouth.net>.
rbb@covalent.net writes:
> I had planned on letting this code work for a few days before making these
> changes, because I wanted to exercise the code before I macro-ized it, but
> I'm flexible.
No, I'm in no hurry to go any further down this path. I don't think
what is there already is worthwhile.
We don't have better performance and we don't need the change in order
to allow modules to implement their own bucket types.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by rb...@covalent.net.
> I already miss the ability to print a bucket in gdb and have it tell
> me in words what the type of the bucket is.
This too can be fixed, by allowing people to add names to the bucket
types, and storing that with the bucket functions. If you would like, as
soon as I finish implementing the changes to ap_get_client_block that were
discussed yesterday, I will make this code use macros and structure
pointers instead of just an index. Then I will make gdb be able to tell
you what kind of bucket it is.
I had planned on letting this code work for a few days before making these
changes, because I wanted to exercise the code before I macro-ized it, but
I'm flexible.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by Jeff Trawick <tr...@bellsouth.net>.
rbb@covalent.net writes:
> On 13 Oct 2000, Jeff Trawick wrote:
> > rbb@locus.apache.org writes:
> > > Log:
> > > Remove all function pointers from the ap_bucket type. These function
> > > pointers are replaced with a global table that allows modules to register
> > > their bucket types. Those bucket types are then allowed to be used in
> > > the server processing. This also required removing all direct calls to
> > > those functions. The ap_bucket type has an index into an array, so in
> > > each ap_bucket_* function, we use that index to find the correct set of
> > > functions.
> >
> > What is the purpose of this? We just got even slower.
>
> The purpose is to shrink the size of the buckets structure so that we
> allocate them faster.
When we use a pool of bucket structures, the cost of allocation will
be unrelated to the size of the bucket structure. I doubt that even
with the current allocation code the current shrinkage results in
faster allocation/deallocation, but that is irrelevant.
> This also divorces the bucket structure from the currently implemented
> bucket types, and makes it possible for people to extend the bucket
> types. Think of an SSL socket for input filtering. We can't use a
> standard socket_bucket, because that would use the wrong recv
> function.
So prior to this patch, if I'm mod_ssl and I create an instance of my
custom bucket type and pass it up, where is the baddness? I need to
make sure that the type doesn't collide with any core-provided types
(a simple ap_get-me-a-type() will take care of that). I need to
provide the proper read() and destroy() and split() functions.
> This has been documented as a necessary enhancement for some time in the
> header files, and doing now keeps us from using e->read or e->split or
> e->destroy, because they won't work at all anymore.
>
> I have already been asked by at least one other person for a way to
> implement their own bucket types that their module understands. This
> allows for that, the previous design didn't, because it used an enumerated
> type for bucket type.
The old mechanism can use enumerated values for core-provided bucket
types and use values above the max for bucket types which have to be
explicitly registered.
I already miss the ability to print a bucket in gdb and have it tell
me in words what the type of the bucket is.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: apache-2.0/src/modules/standard mod_include.c
Posted by rb...@covalent.net.
On 13 Oct 2000, Jeff Trawick wrote:
> rbb@locus.apache.org writes:
> > Log:
> > Remove all function pointers from the ap_bucket type. These function
> > pointers are replaced with a global table that allows modules to register
> > their bucket types. Those bucket types are then allowed to be used in
> > the server processing. This also required removing all direct calls to
> > those functions. The ap_bucket type has an index into an array, so in
> > each ap_bucket_* function, we use that index to find the correct set of
> > functions.
>
> What is the purpose of this? We just got even slower.
The purpose is to shrink the size of the buckets structure so that we
allocate them faster. The problem right now as far as speed is concerned,
is that we are actually using an index into an array rather than a pointer
to another structure. The performance issue can be solved by 1), removing
the functions, and replacing them with macros. 2) using a pointer to a
structure rather than an index into an array.
This also divorces the bucket structure from the currently implemented
bucket types, and makes it possible for people to extend the bucket
types. Think of an SSL socket for input filtering. We can't use a
standard socket_bucket, because that would use the wrong recv function.
This has been documented as a necessary enhancement for some time in the
header files, and doing now keeps us from using e->read or e->split or
e->destroy, because they won't work at all anymore.
I have already been asked by at least one other person for a way to
implement their own bucket types that their module understands. This
allows for that, the previous design didn't, because it used an enumerated
type for bucket type.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------