Re: apr_time_t -- apr_time_usec_t
At 10:14 PM 6/10/2002, Brian Pane wrote: William A. Rowe, Jr. wrote: At 09:31 PM 6/10/2002, I wrote: usec * 1048575 / 100 = busec bsec * 100 / 1048575 = usec whoops, s/1048575/1048576/ That's a major improvement. I was going to complain about the need for both a 64-bit multiplication and a 64-bit division to do the conversion, but 1048576 only requires a shift. In general, I like this design: * It combines most of the benefits of the struct apr_timeval approach with most of the benefits of the current seconds*100+microseconds scalar representation. However, we aught to define convenience macros; #define APR_BUSEC_PER_SEC 1048576 #define APR_TIME_MAKE(sec, usec) (sec * APR_BUSEC_PER_SEC \ + (usec * 100) / APR_USEC_PER_SEC) #define APR_TIME_SEC(time) ((time) / APR_BUSEC_PER_SEC) #define APR_TIME_USEC(time) usec) % APR_BUSEC_PER_SEC) \ * 100) / APR_BUSEC_PER_SEC) * And the transition is easy: just change APR_USEC_PER_SEC to 1048576 and tell all users of APR to recompile. Not quite, simply dividing the time by APR_USEC_PER_SEC would give you seconds, but taking the modulus APR_USEC_PER_SEC wouldn't give you decimal usec. This is why I'd favor using APR_BUSEC_PER_SEC for the name, to prevent new confusion +1 for the binary microseconds implementation -0 for continuing to call it apr_time_t So it's time to invent the proper, new names and give it a whirl. I'll commit such a patch later tonight or so, depending on how things are going. Feel free to beat me to it :-) If we make it conditional on a compile switch, we can benchmark the difference.
Re: apr_time_t -- apr_time_usec_t
On Tue, 11 Jun 2002, William A. Rowe, Jr. wrote: However, we aught to define convenience macros; #define APR_BUSEC_PER_SEC 1048576 I was thinking the same thing. +1. --Cliff
Re: apr_time_t -- apr_time_usec_t
Justin Erenkrantz wrote: Ryan Bloom wrote: This is going to break EVERY apr app out there. I have no problem with the change, but everything is going to be broken. Is there any way to do this by creating an apr_time32_t, or something that will keep the default at 64-bit, and allow people to use 32 if they specify it? How about using the value we already have: typedef apr_int32_t apr_short_interval_time_t; And, since there *are* processors that are 64-bit out there, I'm not entirely sure if it makes sense for us to go back to 32-bit for performance reasons. -- justin I think it's better to maintain the microseconds part, but to split the seconds and microseconds into separate fields in a timeval structure. Sometimes we actually want microseconds. They're useful, for example, when generating unique IDs or when doing custom performance monitoring. Most of the time, in the httpd at least, we just want seconds. We get both for free from gettimeofday(), so why not keep both? Also, apr_short_interval_time_t isn't really a good choice for holding a time_t. That signed 32-bit int is going to overflow in 2038. --Brian
Re: apr_time_t -- apr_time_usec_t
At 05:04 PM 6/10/2002, you wrote: I am tired of seeing this stupid change to the semantics of time_t under Unix continue to cause bugs in every project that uses APR. I must have missed that discussion traveling. Pointers please? apr_time_t must be in seconds. If folks want APR to keep time in microseconds, then they had bloody well change the type name accordingly. apr_time_t must nothing :-) Let's discuss *should(s)* time_t is seconds. I love the idea of apr_time_usec_t and apr_time_sec_t names rather that something as ambigous as apr_time_t (which is misleading, I agree.) As far as adopting apr_time_sec_t throughout, you may be looking forward to your retirement party before a signed 32 bit apr_time_sec_t blows chunks. Having coded against Y2K since 1989, I'd absolutely veto this suggestion for general adoption. Specific cases, fine. I know of one existing bug in httpd that I would consider a showstopper, if I were RM, due to the way APR handles time. In order to fix it, I am going to need to reinstate handling of time in seconds, even if that means abandoning APR's routines. Please share, I'm certain a few more pairs of eyes could prove useful. ++1 on distinguishing apr_time_t to be more meaningful, though!
Re: apr_time_t -- apr_time_usec_t
On Mon, Jun 10, 2002 at 03:57:29PM -0700, Justin Erenkrantz wrote: How about using the value we already have: typedef apr_int32_t apr_short_interval_time_t; Unfortunately, that type still has units of milliseconds. Seems like Roy needs an apr_seconds_t, and apr_short_interval_time_t should be renamed apr_milliseconds_t. -aaron
Re: apr_time_t -- apr_time_usec_t
On Mon, Jun 10, 2002 at 04:14:14PM -0700, Aaron Bannert wrote: Unfortunately, that type still has units of milliseconds. Seems like Roy needs an apr_seconds_t, and apr_short_interval_time_t should be renamed apr_milliseconds_t. er, s/milliseconds/microseconds/ -aaron
Re: apr_time_t -- apr_time_usec_t
On Monday, June 10, 2002, at 04:11 PM, William A. Rowe, Jr. wrote: At 05:04 PM 6/10/2002, you wrote: I am tired of seeing this stupid change to the semantics of time_t under Unix continue to cause bugs in every project that uses APR. I must have missed that discussion traveling. Pointers please? Just do a search for 100 or USEC in the cvs archives. I've seen at least two dozen separate bug fixes applied to APR and httpd that were directly caused by people changing time_t to apr_time_t and expecting it to work. The latest one floated by this weekend. Bad interfaces are evidenced by repetition of the same bugs over time. apr_time_t must be in seconds. If folks want APR to keep time in microseconds, then they had bloody well change the type name accordingly. apr_time_t must nothing :-) Let's discuss *should(s)* No. I will veto the change from time_t to apr_time_t due to misleading changes in semantics. I am going back to the state wherein httpd was robust. Since my change will both reduce bugs and improve performance, without sacrificing any known features, there is sufficient justification for making it in httpd. You can debate how this should effect APR all you like. I'd prefer a solution that is general to APR. I don't know of any APR client apps that make use of microseconds other than flood and ab. time_t is seconds. I love the idea of apr_time_usec_t and apr_time_sec_t names rather that something as ambigous as apr_time_t (which is misleading, I agree.) Fine, that is one solution. As far as adopting apr_time_sec_t throughout, you may be looking forward to your retirement party before a signed 32 bit apr_time_sec_t blows chunks. Having coded against Y2K since 1989, I'd absolutely veto this suggestion for general adoption. Specific cases, fine. time_t is not limited to 32 bits. I never mentioned 32 bits. You must be referring to someone else. Doing 64 bit divides and multiplies are bad for performance, but that is done because we are storing microseconds even though we never use microseconds. Eliminate the microseconds, or move it to a separate field, but keep the 64 bits. I personally think it a waste of time (no pun intended) to worry about a 32bit time_t when time_t is only 32 bits on machines that only have a 32bit interface to their time operations, and hence our clock will still roll-over regardless of how many extra bits we use to store our copy of time_t. I will discuss httpd changes on [EMAIL PROTECTED] -- this thread was just to get a little frustration off my chest regarding APR design decisions that are made based on the theory that they might some day be needed and then never changed (in spite of several requests on my part) based on the theory that they might break some mythical user of APR. It's time to get the clue stick out of the closet. From now on, apr_time_t is considered harmful. Roy
Re: apr_time_t -- apr_time_usec_t
William A. Rowe, Jr. wrote: apr_time_t must be in seconds. If folks want APR to keep time in microseconds, then they had bloody well change the type name accordingly. apr_time_t must nothing :-) Let's discuss *should(s)* time_t is seconds. I love the idea of apr_time_usec_t and apr_time_sec_t names rather that something as ambigous as apr_time_t (which is misleading, I agree.) Agreed. But, IMO, it *is* documented that apr_time_t is microsecond resolution. If people make assumptions then, well, that's bad, but not really a showstopper as far as I'm concerned. Now the nastyness of 64bit mult/division when we (always) need second resolution is another. Sure would be nice if it was an exact power of 2 :) -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
Re: apr_time_t -- apr_time_usec_t
At 09:15 PM 6/10/2002, you wrote: William A. Rowe, Jr. wrote: apr_time_t must be in seconds. If folks want APR to keep time in microseconds, then they had bloody well change the type name accordingly. apr_time_t must nothing :-) Let's discuss *should(s)* time_t is seconds. I love the idea of apr_time_usec_t and apr_time_sec_t names rather that something as ambigous as apr_time_t (which is misleading, I agree.) Agreed. But, IMO, it *is* documented that apr_time_t is microsecond resolution. If people make assumptions then, well, that's bad, but not really a showstopper as far as I'm concerned. Now the nastyness of 64bit mult/division when we (always) need second resolution is another. Sure would be nice if it was an exact power of 2 :) It can be. busec [binary microseconds] has a resolution of 1 / 2^20, leaving 2^41-1 for minutes [and providing for negative time.] That yields 69000-some years forwards or backwards from Jan 1 1970. busec translation would be pretty trivial... time 20 == seconds time (2^20 - 1) == busec (binary microseconds) usec * 1048575 / 100 = busec bsec * 100 / 1048575 = usec The nice thing is that you can do all the addition, subtraction and multiplication you want and come back to seconds/usec values anytime you like. You don't need to perform separate math operations with carry on two separate fields, which is the significant fault to the sec, usec structure case. The question is, how often do you need to convert usec between decimal and binary? Really only to implode or explode the time, either in display/gregorian format or sec/usec structures. Just some boolean food for thought. Bill
Re: apr_time_t -- apr_time_usec_t
At 09:31 PM 6/10/2002, I wrote: usec * 1048575 / 100 = busec bsec * 100 / 1048575 = usec whoops, s/1048575/1048576/
Re: apr_time_t -- apr_time_usec_t
William A. Rowe, Jr. wrote: At 09:31 PM 6/10/2002, I wrote: usec * 1048575 / 100 = busec bsec * 100 / 1048575 = usec whoops, s/1048575/1048576/ That's a major improvement. I was going to complain about the need for both a 64-bit multiplication and a 64-bit division to do the conversion, but 1048576 only requires a shift. In general, I like this design: * It combines most of the benefits of the struct apr_timeval approach with most of the benefits of the current seconds*100+microseconds scalar representation. * It leaves 44 bits of a 64-bit representation free to hold the time_t, so it won't overflow for a few hundred thousand years. * And the transition is easy: just change APR_USEC_PER_SEC to 1048576 and tell all users of APR to recompile. +1 for the binary microseconds implementation -0 for continuing to call it apr_time_t --Brian
Re: apr_time_t -- apr_time_usec_t
On Mon, 10 Jun 2002, Roy T. Fielding wrote: I know of one existing bug in httpd that I would consider a showstopper, if I were RM, due to the way APR handles time. Are you going to tell me what it is? :)
Re: apr_time_t -- apr_time_usec_t
Roy T. Fielding wrote: I am tired of seeing this stupid change to the semantics of time_t under Unix continue to cause bugs in every project that uses APR. apr_time_t must be in seconds. If folks want APR to keep time in microseconds, then they had bloody well change the type name accordingly. +1. I'm tired of taking the perfomance hit of 64-bit divisions on 32-bit CPUs to retrieve the time_t. In order to fix it, though, we'd really need to change the implementation in some way that breaks old code at compile time. Otherwise, if apr_time_t remains a 64-bit int but its meaning changes by a factor of a million, lots of bugs in people's APR-based apps aren't going to show up until runtime. I know of one existing bug in httpd that I would consider a showstopper, if I were RM, due to the way APR handles time. In order to fix it, I am going to need to reinstate handling of time in seconds, even if that means abandoning APR's routines. Don't keep us in suspense...what's the bug?! :-) --Brian
RE: apr_time_t -- apr_time_usec_t
This is going to break EVERY apr app out there. I have no problem with the change, but everything is going to be broken. Is there any way to do this by creating an apr_time32_t, or something that will keep the default at 64-bit, and allow people to use 32 if they specify it? I did a quick search, Dean didn't mention why he made the change to 64-bit back when he committed the code in 2000, and there doesn't seem to have been a conversation about it at the time. Ryan -- Ryan Bloom [EMAIL PROTECTED] 645 Howard St. [EMAIL PROTECTED] San Francisco, CA -Original Message- From: Brian Pane [mailto:[EMAIL PROTECTED] Sent: Monday, June 10, 2002 3:39 PM To: dev@apr.apache.org Subject: Re: apr_time_t -- apr_time_usec_t Roy T. Fielding wrote: I am tired of seeing this stupid change to the semantics of time_t under Unix continue to cause bugs in every project that uses APR. apr_time_t must be in seconds. If folks want APR to keep time in microseconds, then they had bloody well change the type name accordingly. +1. I'm tired of taking the perfomance hit of 64-bit divisions on 32-bit CPUs to retrieve the time_t. In order to fix it, though, we'd really need to change the implementation in some way that breaks old code at compile time. Otherwise, if apr_time_t remains a 64-bit int but its meaning changes by a factor of a million, lots of bugs in people's APR-based apps aren't going to show up until runtime. I know of one existing bug in httpd that I would consider a showstopper, if I were RM, due to the way APR handles time. In order to fix it, I am going to need to reinstate handling of time in seconds, even if that means abandoning APR's routines. Don't keep us in suspense...what's the bug?! :-) --Brian
Re: apr_time_t -- apr_time_usec_t
Ryan Bloom wrote:
This is going to break EVERY apr app out there. I have no problem with
the change, but everything is going to be broken. Is there any way to
do this by creating an apr_time32_t, or something that will keep the
default at 64-bit, and allow people to use 32 if they specify it?
How about leaving apr_time_t unchanged and adding an apr_timeval:
typedef struct {
unsigned tv_sec;
unsigned tv_usec;
} apr_timeval;
plus
apr_status_t apr_timeval_now(apr_timeval* time)
to complement (not replace) the existing apr_time_now()
and apr_timeval versions of other time functions.
--Brian
Re: apr_time_t -- apr_time_usec_t
Ryan Bloom wrote: This is going to break EVERY apr app out there. I have no problem with the change, but everything is going to be broken. Is there any way to do this by creating an apr_time32_t, or something that will keep the default at 64-bit, and allow people to use 32 if they specify it? How about using the value we already have: typedef apr_int32_t apr_short_interval_time_t; And, since there *are* processors that are 64-bit out there, I'm not entirely sure if it makes sense for us to go back to 32-bit for performance reasons. -- justin
