> The patch looks great. It covers everything I wanted to see when I
> thought about writing it. :) I'll test it a bit and hopefully commit
> it tomorrow. I like TCP_CORK though, so I want to uncomment those
> setsockopts.
>
> Hopefully, we can put this into APR at some point.
Sounds great. I actually have been wondering, though, about the
interface for the sendfile method in ap_iol_methods. I used the one that
already existed in ap_iol.h (in the include dir) and figured that some of
the strange arguments were necessary on Win32 for its TransmitFile
implementation. Here's the declaration:
"ap_status_t (*sendfile)(ap_iol *fd, ap_file_t *file, long size, const
char* header, long hdrlen, int flags)"
However, the only invocation of this method comes in http_protocol as:
"iol_sendfile( r->connection->client->iol, fd, len, NULL, 0, 0 )" (line
2013)
Actually, I can't see anywhere in win32_sendfile where header, hdrlen, or
flags are used. I also haven't used those args in the Unix version,
because default_handler, at least, calls "ap_send_http_header" before
calling ap_send_fd. The current implementation also leaves a #ifdef
HAVE_SENDFILE in ap_send_fd.
So I think this leaves us with a few possibilities to consider:
- Eliminate the #ifdefs in the main code by abstracting in APR. This
would mean using ap_sendfile in ap_send_fd and providing a new
implementation of ap_sendfile for OSes that don't have the actual system
call. That would be a simple, standard read/write loop. This seems like
a necessity to allow APR to truly provide the same API on all platforms.
- Either eliminate the header-related arguments or use them in the
default_handler. There could actually be some good reasons for
perfomance to have the sendfile method send the headers (particularly by
setting the sockopt TCP_CORK on Linux), and I assume this is the reason
why those args are there in the first place. However, we'd need to
implement an ap_get_http_reader function to do exactly what
ap_send_http_header currently does, but return a string instead of
actually sending it. This might come in handy for other modules or
performance tweaks that want to send the header in their own way.
- It seems to me that sendfile() and mmapped files are mutually exclusive
strategies to quickly transmit an entire file. If we moved the mmap code
from default_handler to the new ap_sendfile, it would eliminate a lot of
#ifdef'd cruft from the main module. This would put all the
platform-specific performance tweaks into APR where they, IMHO, probably
should be. We can always provide a NO_MMAP flag for ap_sendfile that
ensures no memory mapping will be used, if in some case it would be
undesirable.
- Both Linux and FreeBSD allow you to specify an offset into the file in
the sendfile() API. I don't believe that Win32 TransmitFile() allows
this. It would allow ap_send_fd_length to be implemented with sendfile
as well. How often in practice is ap_send_fd_length really used to
transmit files of a decent size? I have no idea whether this change
would make a noticeable performance difference, so it probably isn't
worth doing unless Win32 does have some hidden offset specification.
I think that these little changes could clean up http_core and the
default handler significantly, while also making APR more useful and
improving performance. I'd be glad to do the patches myself, if you all
think it's a reasonable strategy.
By the way, I've heard that HP-UX also has a sendfile() call. Is that
accurate? Any other platforms?
Finally, I'd like to put in a vote for the "option 1" method of
incorporating iol_socket into APR (the non-linked-list version). It
seems much simpler and I don't really see a performance difference.
Thanks, folks,
--JRZ
|