apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ryan Bloom" <...@covalent.net>
Subject RE: more notes on the apr_time_t issue
Date Mon, 15 Jul 2002 02:54:23 GMT
> From: William A. Rowe, Jr. [mailto:wrowe@rowe-clan.net]
> 
> At 03:21 PM 7/14/2002, Ryan Bloom wrote:
>
> >So my names are wrong, but the concept is the same.  More macros than
> >you can keep straight to fill in the time structure.  The advantage
to
> >the current model, is that there is one macro:  APR_SEC_FROM_USEC.
> 
> No, that's not entirely fair, and no, we can have an APR_TIME_PER_SEC
> value.  I'm not suggesting we hide this scalar behind a curtain.  The
> other
> macros are for convenience, not to be a pain in the ass.  Others
disagree,
> but once we hit binary compatibility, these can never change.
> 
> The macros, however, are considerably faster on all platforms, they
don't
> rely on the compiler doing all the optimization.  That's why the
macros
> are
> infinitely preferable to the constant.  They are [will also be] vetted
so
> that
> the correct behavior is on the library, not the developer's math.

The macros are no faster than the constant.  The implementation is
faster.  Completely different.

> >In fact, you added one more macro, the nsec variant, so now you have
> >_10_ macros to get information out of a simple time type.  That
doesn't
> >seem incredibly extreme to you?
> 
> Not in terms of performance.  We have a four real macros;
> 
> apr_time_sec_get(time) [which is the same as...]
> apr_time_to_sec(time)
> apr_time_from_sec(sec)
> 
> apr_time_xsec_get(time) [gets only the fraction of a second]
> apr_time_to_xsec(time) [converts complete value by scale]
> apr_time_from_xsec(xsec) [converts complete value by scale]
> apr_time_xmake(sec, xsec);
> 
> So xsec can be an nsec, usec or msec, depending on the app.
> So WHAT?  They all follow an incredibly simple pattern.

BTW, nsec is completely bogus.  There isn't an OS that I know of that
reports nsec intervals.  Windows comes the closest with 100 nsec chunks,
but that isn't nsec.  Having a macro that purports to report nsecs when
NONE of our OSes can accurately report them is bogus.

Four macros, by four different scales, equals 16 macros.  And, at the
end of the day, the values stored are useful once you leave APR.

BTW, the apr_time_xmake(sec, xsec) macro is completely wrong IMHO.  In
order to use it, I need to provide multiple values, but that doesn't
make any sense, because we have four time scales.  Either provide a
single value, or four.  Two just doesn't make sense.

> I have no problem fixing the ambiguity between _xsec_get and _to_xsec,
> since the difference is subtle.  Perhaps apr_time_frac_to_xsec(time)
> instead of _xsec_get?  (saying, only the fractional portion of a
second?)

Perhaps an example of the difference, because I don't see it.

> > > >22 functions/macros to deal with time. 8 of those are because we
are
> > > >storing time in a format that nobody else expects.
> > >
> > > We have those today.  We have them because we deal in uniform
> > > apr_time_t RIGHT NOW TODAY.  This whole proposal changes nothing
> > > in that department.
> >
> >Sure it does.  We have 1 macro today, now we need 10!
> 
> Quit it with the ten bs :)  Ok, four macros of three-four flavors,
instead
> of
> one macro const value.  We've been using those macros for several
weeks,
> or had you missed that?
> 
> There is no other impact in complexity here, IMHO.

Quite honestly, I have tried to ignore the macro-ization that has been
done recently, although whenever I see ten commits in a row to implement
them, I do curse loudly.  I would never use those macros with today's
implementation, because they just get in the way of me knowing what is
going on.

> >great, that means that EVERY single call that deals with time in
EVERY
> >single APR app needs to change.  And, it means that calling those
> >functions will just become a painful exercise for new programmers.
> >"Let's see, I need to pass an apr_time value, which is a binary usec
> >implementation, now, how do I get binary usec's again?  I guess I'll
go
> >read the docs to find out."  Programming used to make sense, because
the
> >types made sense and for simple things, like timeouts, programmers
could
> >use simple values.  Now, our timeouts are incredibly complex, which
> >makes using them painful.   :-(
> 
> No, this is _NO_ more difficult than going back to the docs to figure
out
> what APR_FOO_FLAG values you must use.  Look at any sources, and
> you will see examples (including apr's own code, once Brian and I are
> finished cleaning up.)

You're right, it is no more difficult, it is equally as difficult.  My
complaint is that the concept of time is FAR simpler than file or
network operations.  I don't mind complex API's for complex problems,
but time is a simple complex.  It is an offset from a set point in time,
or an offset from an arbitrary point in time.  Why is such a simple
concept causing such a complex solution?

> > >    2. we already have this issue on win32, and I don't like where
we
> > are right now.
> > >       Win32 is using msec, but we pass and return usec values.
That's
> > just sure
> > >       to introduce bugs.  I'm going to cache *two* values in the
> > socket_t structure,
> >
> > >       s->timeout and s->timeout_ms.  This makes state changes
quick,
> but
> > >       keeps the data readily available for recall with
getsockopt().
> >
> >And guarantees bugs.
> 
> Wrong.  These are internals.  NOT externals.  This particular value is
set
> in one
> place today (for sockets) ... apr_set_sockopt(...APR_SO_TIMEOUT...)
...
> and
> I will
> assure it doesn't happen anywhere else.

If it is only happening in one place, then only store it in one format.
I don't care if it is internals or externals either.  Data duplication
is a bad idea unless the performance improvement is massive.  Every
other place we have had this kind of data duplication in APR before, it
has caused bugs.  And yes, the original implementation for APR's time
code had this kind of duplication on Windows to solve the exact problem
that you are trying to solve.

> >:-(  Caching speeds things up, but it is almost
> >guaranteed to cause somebody to update 1 of them without updating the
> >other at some point in the future.
> 
> Not when apr_socket_t is opaque.

It does the day that you don't pay attention to a commit because you are
busy, and a new developer makes a change to the code.  Someday, you will
either move on to a new project or not review a commit, and a bug will
be committed.  I am willing to stake my reputation on it.  (Not sure
what that is worth today, but there it is).

> >And has the same problem, that in the name of performance, you are
going
> >to introduce bugs.
> 
> No, only if the APR authors don't pay attention.  Which we do.  This
is a
> reason
> you don't do mass renames with perl scripts - you look at the code,
> discover any
> existing bugs, and then apply the best new code.

Funny, but we have done multiple mass-renames with Perl code in the
past.

> >I am simply suggesting nobody has weighed in from the user's point of
> >view, other than to say "We must have usec resolution".  The API that
is
> >currently being discussed is big and complex.  I would personally
never
> >write a program using APR if a concept as simple as time required
this
> >much work to get into my program.  This API would literally stop me
from
> >using APR at all, because of how often I would need to read the docs
on
> >how to use the API.
> 
> If this is too complex, you better quit using apr_socket's,
apr_file's,
> and a whole lot more complicated types with lots 'o functions.

Again, complex solutions for complex problems, simple solutions for
simple problems.  We have just designed an incredibly complex solution
for a very simple problem.

> > > >Maybe would could have had:
> > > >
> > > >#ifndef NEW_TIME_FMT
> > > >#error "Please convert you code to use busec's
> > > >#endif
> > >
> > > Interesting option :-)  But not entirely practical, I think.
> > > It doesn't take into account the developer's own header files
> > > that might contain time constants.
> >
> >Sure it does.  It is just saying that if you include the apr_time.h
> >header, you will have to define an APR specific macro to prove that
you
> >are using the new definition of apr_time_t.
> 
> I didn't dismiss it, just suggesting it's a pretty obtuse versioning
> scheme.
> How many will we end up with like this?

How many times will we have vetos based on type names?  Obviously the
macro wouldn't last forever, just long enough to resolve the veto.  It
is a technical hack to get around a veto, nothing more.

> I don't mean to sound harsh here, but we've been working in this
direction
> for three weeks or so, much longer than the current debate.  I'm just
> asking
> myself where you've been while Brian Pane and I were introducing the
new
> macros and fixing up the code in preparation for initial profiling and
> whatever
> potential change.

For the most part, I have been busy and not watching too closely.  When
the macros were added, I assumed I could ignore them.  Now I can't,
because they are the only way that the time code can work.

Ryan



Mime
View raw message