You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Brian Havard <br...@kheldar.apana.org.au> on 2000/06/11 15:07:28 UTC
Buffered file I/O & thread safety
Some benchmarking clearly shows (at least on OS/2, I expect other platforms
to be similar) that the mutex lock used to make the buffered file I/O
thread safe is quite expensive (a lock & unlock for every byte read in some
cases). It is also usually not necessary. The only situation it's needed is
when the same ap_file_t object is used in multiple threads (EG a log file
used by all threads).
In the case of mod_include or config file reading only 1 thread will ever
touch the ap_file_t so a serious speed boost can be achieved by turning off
the locking. To this end I propose adding a flag value for ap_open() to
indicate if the handle needs to be thread safe, say APR_MT.
Any objections?
--
______________________________________________________________________________
| Brian Havard | "He is not the messiah! |
| brianh@kheldar.apana.org.au | He's a very naughty boy!" - Life of Brian |
------------------------------------------------------------------------------
Re: Buffered file I/O & thread safety
Posted by rb...@covalent.net.
> > Err, considering that the calls actuall affected are ap_read & ap_write
> > that would require finding & changing all occurances of those calls. I much
> > prefer a single flag change to ap_open. It aint 'Tons' of flag checking,
> > just one.
>
> There's lots of flag checking for APR_READ/WRITE/etc., APR_OS_DEFAULT,
> DELONCLOSE, APR_BUFFERED, etc. This is an added flag.
This doesn't address any of what is in the previous message. If we make
the locking/no-locking thing a separate function that basically calls
ap_read/ap_write, we have to find and consider all of the calls
ap_read/ap_write to determine if they should use ap_read/ap_write or
ap_fread/ap_fwrite. With adding one flag to ap_open, we just have to find
and fix the calls to ap_open.
Yes, ap_open does a lot of flag checking. This is required for
portability. It is a PITA, but it is required. If you can find a way to
remain portable and avoid all of the flag checking, I would love to hear
it.
> The important part isn't readability of the patch, but readability of
> the entire function.
>
> ap_open is 75 lines long (generously spaced, but still...). This is
> all overhead that is required, both for a person trying to parse the
> code and for the actual running of the code, and none of this actually
> includes the OS's work of opening a file. ap_open isn't as hard to
> read as ap_read, yet.
The entire ap_open function is relatively clean. We check a bunch of
flags, and open the file. I haven't seen a suggestion for how to clean up
the ap_open function yet. Every flag that has been added to ap_open has
been added to support a specific platform. I do not believe we are
talking about a great deal of overhead. I can see some places that we can
cleanup some of the overhead of the checks, but not much.
Remember that most of APR has never been optimized. The goal was to get
APR working, and optimize it later. I expect APR to be radically changed
(the source not the API's) for performance sometime between 2.0.0 and
2.0.1. This is not an optimization that needs to be done before 2.0.0 is
actually release IMHO.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: Buffered file I/O & thread safety
Posted by Manoj Kasichainula <ma...@io.com>.
On Mon, Jun 12, 2000 at 10:12:32PM +1000, Brian Havard wrote:
> Err, considering that the calls actuall affected are ap_read & ap_write
> that would require finding & changing all occurances of those calls. I much
> prefer a single flag change to ap_open. It aint 'Tons' of flag checking,
> just one.
There's lots of flag checking for APR_READ/WRITE/etc., APR_OS_DEFAULT,
DELONCLOSE, APR_BUFFERED, etc. This is an added flag.
> Is this so hard to read?
>
> --- lib/apr/file_io/os2/readwrite.c 2000/05/27 18:01:00 1.27
> +++ lib/apr/file_io/os2/readwrite.c 2000/06/11 07:01:44
The important part isn't readability of the patch, but readability of
the entire function.
ap_open is 75 lines long (generously spaced, but still...). This is
all overhead that is required, both for a person trying to parse the
code and for the actual running of the code, and none of this actually
includes the OS's work of opening a file. ap_open isn't as hard to
read as ap_read, yet.
Re: Buffered file I/O & thread safety
Posted by Brian Havard <br...@kheldar.apana.org.au>.
On Mon, 12 Jun 2000 09:31:49 -0400, Jeff Trawick wrote:
>> From: "Brian Havard" <br...@kheldar.apana.org.au>
>> Date: Mon, 12 Jun 2000 22:12:32 +1000 (EST)
>>
>> Err, considering that the calls actuall affected are ap_read & ap_write
>> that would require finding & changing all occurances of those calls. I much
>> prefer a single flag change to ap_open. It aint 'Tons' of flag checking,
>> just one. Is this so hard to read?
>
>We need the APR_HAS_THREADS check, right?
The unix code already has it (that was the OS/2 code). I guess it would
help in the rare case of having threads disabled on OS/2.
--
______________________________________________________________________________
| Brian Havard | "He is not the messiah! |
| brianh@kheldar.apana.org.au | He's a very naughty boy!" - Life of Brian |
------------------------------------------------------------------------------
Re: Buffered file I/O & thread safety
Posted by Jeff Trawick <tr...@bellsouth.net>.
> From: "Brian Havard" <br...@kheldar.apana.org.au>
> Date: Mon, 12 Jun 2000 22:12:32 +1000 (EST)
>
> Err, considering that the calls actuall affected are ap_read & ap_write
> that would require finding & changing all occurances of those calls. I much
> prefer a single flag change to ap_open. It aint 'Tons' of flag checking,
> just one. Is this so hard to read?
We need the APR_HAS_THREADS check, right?
>
> --- lib/apr/file_io/os2/readwrite.c 2000/05/27 18:01:00 1.27
> +++ lib/apr/file_io/os2/readwrite.c 2000/06/11 07:01:44
> @@ -77,7 +77,8 @@
> ULONG blocksize;
> ULONG size = *nbytes;
>
> - ap_lock(thefile->mutex);
#if APR_HAS_THREADS
> + if (thefile->flags & APR_MT)
> + ap_lock(thefile->mutex);
#endif
>
> if (thefile->direction == 1) {
> ap_flush(thefile);
--
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: Buffered file I/O & thread safety
Posted by rb...@covalent.net.
> >Tons of flag checking can make the code harder to read IMO. What the
> >Linux kernel does is to provide an unlocked version of a call, and
> >then make the locked version wrap that.
>
> Err, considering that the calls actuall affected are ap_read & ap_write
> that would require finding & changing all occurances of those calls. I much
> prefer a single flag change to ap_open. It aint 'Tons' of flag checking,
> just one. Is this so hard to read?
>
> --- lib/apr/file_io/os2/readwrite.c 2000/05/27 18:01:00 1.27
> +++ lib/apr/file_io/os2/readwrite.c 2000/06/11 07:01:44
> @@ -77,7 +77,8 @@
> ULONG blocksize;
> ULONG size = *nbytes;
>
> - ap_lock(thefile->mutex);
> + if (thefile->flags & APR_MT)
> + ap_lock(thefile->mutex);
>
> if (thefile->direction == 1) {
> ap_flush(thefile);
+1.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: Buffered file I/O & thread safety
Posted by Brian Havard <br...@kheldar.apana.org.au>.
On Sun, 11 Jun 2000 15:11:11 -0500, Manoj Kasichainula wrote:
>On Sun, Jun 11, 2000 at 11:07:28PM +1000, Brian Havard wrote:
>> In the case of mod_include or config file reading only 1 thread will ever
>> touch the ap_file_t so a serious speed boost can be achieved by turning off
>> the locking. To this end I propose adding a flag value for ap_open() to
>> indicate if the handle needs to be thread safe, say APR_MT.
>
>Tons of flag checking can make the code harder to read IMO. What the
>Linux kernel does is to provide an unlocked version of a call, and
>then make the locked version wrap that.
Err, considering that the calls actuall affected are ap_read & ap_write
that would require finding & changing all occurances of those calls. I much
prefer a single flag change to ap_open. It aint 'Tons' of flag checking,
just one. Is this so hard to read?
--- lib/apr/file_io/os2/readwrite.c 2000/05/27 18:01:00 1.27
+++ lib/apr/file_io/os2/readwrite.c 2000/06/11 07:01:44
@@ -77,7 +77,8 @@
ULONG blocksize;
ULONG size = *nbytes;
- ap_lock(thefile->mutex);
+ if (thefile->flags & APR_MT)
+ ap_lock(thefile->mutex);
if (thefile->direction == 1) {
ap_flush(thefile);
--
______________________________________________________________________________
| Brian Havard | "He is not the messiah! |
| brianh@kheldar.apana.org.au | He's a very naughty boy!" - Life of Brian |
------------------------------------------------------------------------------
Re: Buffered file I/O & thread safety
Posted by Manoj Kasichainula <ma...@io.com>.
On Mon, Jun 12, 2000 at 04:24:21AM -0700, dean gaudet wrote:
> this is one of those areas where i think that APR is too much, and isn't
> the minimum it could be.
>
> i've said before there's a reason for having both open/read/write/close
> and having fopen/fread/fwrite/fclose.
+1
On Mon, Jun 12, 2000 at 10:22:31PM +1000, Brian Havard wrote:
> And that reason is? I think it's much simpler to have one configurable open
> function. That way you can switch features on & off without having to
> modify every use of the handle to use the other function call (open/fopen,
> read/fread etc).
ap_{read,write} are basically a big "if (thefile->buffered)" block,
with two big hunks, once called with the flag set and the other called
with the flag unset. That *screams* out for two seperate functions.
If shielding the basic difference between these two behaviors is
important, I'd suggest using IOLs for everything like Ryan discussed,
and making this buffering code an IOL.
Re: Buffered file I/O & thread safety
Posted by Brian Havard <br...@kheldar.apana.org.au>.
On Mon, 12 Jun 2000 04:24:21 -0700 (PDT), dean gaudet wrote:
>On Sun, 11 Jun 2000 rbb@covalent.net wrote:
>
>> No, this is a flag that will need to be set on the open call. The open
>> basically does nothing but checking flags. I dislike thoroughly having
>> multiple versions of the same functions just to not have mutex locking.
>
>i dislike having tonnes of flag checking. i don't see the point in
>supporting mutexes.
>
>it's really unfortunate, for example, that logging is going to require a
>userland mutex...
Only if APR_BUFFERED is turned on, something we most likely won't do for
logs.
>when on unix an O_APPEND file has atomicity provided by
>the kerenl already...
And this is assumed to be true in non-buffered mode. A mutex is only used
in buffered mode.
>so any userland locks are merely Yet More cache-line
>activity which isn't required. (i don't know about other operating
>systems, but i'd be surprised if unix is alone with this feature.)
It's not.
>this is one of those areas where i think that APR is too much, and isn't
>the minimum it could be.
>
>i've said before there's a reason for having both open/read/write/close
>and having fopen/fread/fwrite/fclose.
And that reason is? I think it's much simpler to have one configurable open
function. That way you can switch features on & off without having to
modify every use of the handle to use the other function call (open/fopen,
read/fread etc).
--
______________________________________________________________________________
| Brian Havard | "He is not the messiah! |
| brianh@kheldar.apana.org.au | He's a very naughty boy!" - Life of Brian |
------------------------------------------------------------------------------
Re: Buffered file I/O & thread safety
Posted by dean gaudet <dg...@arctic.org>.
On Sun, 11 Jun 2000 rbb@covalent.net wrote:
> No, this is a flag that will need to be set on the open call. The open
> basically does nothing but checking flags. I dislike thoroughly having
> multiple versions of the same functions just to not have mutex locking.
i dislike having tonnes of flag checking. i don't see the point in
supporting mutexes.
it's really unfortunate, for example, that logging is going to require a
userland mutex... when on unix an O_APPEND file has atomicity provided by
the kerenl already... so any userland locks are merely Yet More cache-line
activity which isn't required. (i don't know about other operating
systems, but i'd be surprised if unix is alone with this feature.)
this is one of those areas where i think that APR is too much, and isn't
the minimum it could be.
i've said before there's a reason for having both open/read/write/close
and having fopen/fread/fwrite/fclose.
-dean
Re: Buffered file I/O & thread safety
Posted by rb...@covalent.net.
> > In the case of mod_include or config file reading only 1 thread will ever
> > touch the ap_file_t so a serious speed boost can be achieved by turning off
> > the locking. To this end I propose adding a flag value for ap_open() to
> > indicate if the handle needs to be thread safe, say APR_MT.
>
> Tons of flag checking can make the code harder to read IMO. What the
> Linux kernel does is to provide an unlocked version of a call, and
> then make the locked version wrap that.
>
> Then it also becomes extremely clear that you're doing something
> unsafe, as OtherBill requested.
No, this is a flag that will need to be set on the open call. The open
basically does nothing but checking flags. I dislike thoroughly having
multiple versions of the same functions just to not have mutex locking.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: Buffered file I/O & thread safety
Posted by Manoj Kasichainula <ma...@io.com>.
On Sun, Jun 11, 2000 at 11:07:28PM +1000, Brian Havard wrote:
> In the case of mod_include or config file reading only 1 thread will ever
> touch the ap_file_t so a serious speed boost can be achieved by turning off
> the locking. To this end I propose adding a flag value for ap_open() to
> indicate if the handle needs to be thread safe, say APR_MT.
Tons of flag checking can make the code harder to read IMO. What the
Linux kernel does is to provide an unlocked version of a call, and
then make the locked version wrap that.
Then it also becomes extremely clear that you're doing something
unsafe, as OtherBill requested.
RE: Buffered file I/O & thread safety
Posted by "William A. Rowe, Jr." <wr...@lnd.com>.
> From: Greg Stein [mailto:gstein@lyra.org]
> Sent: Sunday, June 11, 2000 2:27 PM
>
> On Sun, Jun 11, 2000 at 11:07:28PM +1000, Brian Havard wrote:
> > Some benchmarking clearly shows (at least on OS/2, I expect
> other platforms
> > to be similar) that the mutex lock used to make the
> buffered file I/O
> > thread safe is quite expensive (a lock & unlock for every
> byte read in some
> > cases). It is also usually not necessary. The only
> situation it's needed is
> > when the same ap_file_t object is used in multiple threads
> (EG a log file
> > used by all threads).
> >
> > In the case of mod_include or config file reading only 1
> thread will ever
> > touch the ap_file_t so a serious speed boost can be
> achieved by turning off
> > the locking. To this end I propose adding a flag value for
> ap_open() to
> > indicate if the handle needs to be thread safe, say APR_MT.
> >
> > Any objections?
>
> Sounds like a good plan. Default should be "safe", meaning
> locking. Add the APR_MT to get the speed.
Uh... then let's try APR_BUFF_NO_MTLOCK or something MUCH
clearer that they are doing something potentially bad :-)
Re: Buffered file I/O & thread safety
Posted by Brian Havard <br...@kheldar.apana.org.au>.
On Sun, 11 Jun 2000 12:26:38 -0700, Greg Stein wrote:
>On Sun, Jun 11, 2000 at 11:07:28PM +1000, Brian Havard wrote:
>> Some benchmarking clearly shows (at least on OS/2, I expect other platforms
>> to be similar) that the mutex lock used to make the buffered file I/O
>> thread safe is quite expensive (a lock & unlock for every byte read in some
>> cases). It is also usually not necessary. The only situation it's needed is
>> when the same ap_file_t object is used in multiple threads (EG a log file
>> used by all threads).
>>
>> In the case of mod_include or config file reading only 1 thread will ever
>> touch the ap_file_t so a serious speed boost can be achieved by turning off
>> the locking. To this end I propose adding a flag value for ap_open() to
>> indicate if the handle needs to be thread safe, say APR_MT.
>>
>> Any objections?
>
>Sounds like a good plan. Default should be "safe", meaning locking. Add the
>APR_MT to get the speed.
Ok, but then the flag would be, say, APR_ST (single thread).
--
______________________________________________________________________________
| Brian Havard | "He is not the messiah! |
| brianh@kheldar.apana.org.au | He's a very naughty boy!" - Life of Brian |
------------------------------------------------------------------------------
Re: Buffered file I/O & thread safety
Posted by Greg Stein <gs...@lyra.org>.
On Sun, Jun 11, 2000 at 11:07:28PM +1000, Brian Havard wrote:
> Some benchmarking clearly shows (at least on OS/2, I expect other platforms
> to be similar) that the mutex lock used to make the buffered file I/O
> thread safe is quite expensive (a lock & unlock for every byte read in some
> cases). It is also usually not necessary. The only situation it's needed is
> when the same ap_file_t object is used in multiple threads (EG a log file
> used by all threads).
>
> In the case of mod_include or config file reading only 1 thread will ever
> touch the ap_file_t so a serious speed boost can be achieved by turning off
> the locking. To this end I propose adding a flag value for ap_open() to
> indicate if the handle needs to be thread safe, say APR_MT.
>
> Any objections?
Sounds like a good plan. Default should be "safe", meaning locking. Add the
APR_MT to get the speed.
+1
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: Buffered file I/O & thread safety
Posted by rb...@covalent.net.
> Some benchmarking clearly shows (at least on OS/2, I expect other platforms
> to be similar) that the mutex lock used to make the buffered file I/O
> thread safe is quite expensive (a lock & unlock for every byte read in some
> cases). It is also usually not necessary. The only situation it's needed is
> when the same ap_file_t object is used in multiple threads (EG a log file
> used by all threads).
>
> In the case of mod_include or config file reading only 1 thread will ever
> touch the ap_file_t so a serious speed boost can be achieved by turning off
> the locking. To this end I propose adding a flag value for ap_open() to
> indicate if the handle needs to be thread safe, say APR_MT.
>
> Any objections?
+1.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------