You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Brandon Ehle <az...@yahoo.com> on 2002/12/11 05:46:53 UTC
[PATCH] svn-fast-time-from-cstring.patch
* subversion/libsvn_subr/time.c(svn_time_from_cstring):
Unroll the sscanf when converting time from a string for
better performance.
Index: subversion/libsvn_subr/time.c
===================================================================
--- subversion/libsvn_subr/time.c (revision 3971)
+++ subversion/libsvn_subr/time.c (working copy)
@@ -142,44 +142,53 @@
return -1;
}
-
svn_error_t *
svn_time_from_cstring(apr_time_t *when, const char *data, apr_pool_t *pool)
{
apr_time_exp_t exploded_time;
apr_status_t apr_err;
char wday[4], month[4];
+ const char *c = data;
- /* First try the new timestamp format. */
- if (sscanf (data,
- timestamp_format,
- &exploded_time.tm_year,
- &exploded_time.tm_mon,
- &exploded_time.tm_mday,
- &exploded_time.tm_hour,
- &exploded_time.tm_min,
- &exploded_time.tm_sec,
- &exploded_time.tm_usec) == 7)
- {
- exploded_time.tm_year -= 1900;
- exploded_time.tm_mon -= 1;
- exploded_time.tm_wday = 0;
- exploded_time.tm_yday = 0;
- exploded_time.tm_isdst = 0;
- exploded_time.tm_gmtoff = 0;
-
- apr_err = apr_implode_gmt (when, &exploded_time);
- if(apr_err != APR_SUCCESS)
- {
- return svn_error_createf (SVN_ERR_BAD_DATE, apr_err, NULL,
- "Date conversion failed.");
- }
-
- return SVN_NO_ERROR;
- }
- /* Then try the compatibility option. */
- else if (sscanf (data,
- old_timestamp_format,
+ /* Open-code parsing of the new timestamp format, as this
+ is a hot path for reading the entries file.
+
+ This format looks like:
+ "2001-08-31T04:24:14.966996Z" */
+ exploded_time.tm_year = strtoul(c, (char**)&c, 10);
+ if (*c++ != '-') goto fail;
+ exploded_time.tm_mon = strtoul(c, (char**)&c, 10);
+ if (*c++ != '-') goto fail;
+ exploded_time.tm_mday = strtoul(c, (char**)&c, 10);
+ if (*c++ != 'T') goto fail;
+ exploded_time.tm_hour = strtoul(c, (char**)&c, 10);
+ if (*c++ != ':') goto fail;
+ exploded_time.tm_min = strtoul(c, (char**)&c, 10);
+ if (*c++ != ':') goto fail;
+ exploded_time.tm_sec = strtoul(c, (char**)&c, 10);
+ if (*c++ != '.') goto fail;
+ exploded_time.tm_usec = strtoul(c, (char**)&c, 10);
+ if (*c++ != 'Z') goto fail;
+
+ exploded_time.tm_year -= 1900;
+ exploded_time.tm_mon -= 1;
+ exploded_time.tm_wday = 0;
+ exploded_time.tm_yday = 0;
+ exploded_time.tm_isdst = 0;
+ exploded_time.tm_gmtoff = 0;
+
+ apr_err = apr_implode_gmt(when, &exploded_time);
+ if (apr_err == APR_SUCCESS)
+ return SVN_NO_ERROR;
+
+ return svn_error_createf(SVN_ERR_BAD_DATE, apr_err, NULL,
+ "Date conversion failed.");
+
+fail:
+ /* Try the compatibility option. This does not need to be fast,
+ as this format is no longer generated and the client will convert
+ an old-format entries file the first time it reads it. */
+ if (sscanf(data, old_timestamp_format,
wday,
&exploded_time.tm_mday,
month,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Philip Martin <ph...@codematters.co.uk>.
Brandon Ehle <az...@yahoo.com> writes:
> >If I look at /usr/local/apache2/include/httpd.h it says
> >
> > /** strtoul does not exist on sunos4. */
> > #ifdef strtoul
> > #undef strtoul
> > #endif
> > #define strtoul strtoul_is_not_a_portable_function_use_strtol_instead
> >
> >Perhaps we should use strtol into a temporary, and check for -ve
> >values.
>
> Doesn't matter either way because it won't overrun for millions of
> years,
I was thinking more of rejecting ill-formed input such as
2002--9--6T01:05:05.000000
I don't suppose it matters if apr_implode_time will reject it.
> I had just read somewhere that strtoul was more portable that
> strtol.
We already use strtol, we don't use strtoul.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Brandon Ehle <az...@yahoo.com>.
>
>
>If I look at /usr/local/apache2/include/httpd.h it says
>
> /** strtoul does not exist on sunos4. */
> #ifdef strtoul
> #undef strtoul
> #endif
> #define strtoul strtoul_is_not_a_portable_function_use_strtol_instead
>
>Perhaps we should use strtol into a temporary, and check for -ve
>values.
>
>
Doesn't matter either way because it won't overrun for millions of
years, I had just read somewhere that strtoul was more portable that strtol.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Michael Price <mp...@atl.lmco.com>.
Philip Martin writes:
> If I look at /usr/local/apache2/include/httpd.h it says
>
> /** strtoul does not exist on sunos4. */
> #ifdef strtoul
> #undef strtoul
> #endif
> #define strtoul strtoul_is_not_a_portable_function_use_strtol_instead
>
> Perhaps we should use strtol into a temporary, and check for -ve
> values.
>
> > + exploded_time.tm_mon = strtoul(c, (char**)&c, 10);
> > + if (*c++ != '-') goto fail;
> > + exploded_time.tm_mday = strtoul(c, (char**)&c, 10);
> > + if (*c++ != 'T') goto fail;
> > + exploded_time.tm_hour = strtoul(c, (char**)&c, 10);
> > + if (*c++ != ':') goto fail;
> > + exploded_time.tm_min = strtoul(c, (char**)&c, 10);
> > + if (*c++ != ':') goto fail;
> > + exploded_time.tm_sec = strtoul(c, (char**)&c, 10);
> > + if (*c++ != '.') goto fail;
> > + exploded_time.tm_usec = strtoul(c, (char**)&c, 10);
> > + if (*c++ != 'Z') goto fail;
I now see why Kirk McKusick refers to code optimization as code
"uglification". His use of "uglify" in place of optimize is even
more humorous after seeing examples like this.
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2002-12-11 at 10:45, Philip Martin wrote:
> /** strtoul does not exist on sunos4. */
> Perhaps we should use strtol into a temporary, and check for -ve
> values.
SunOS 4? You're kidding, right?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Philip Martin <ph...@codematters.co.uk>.
Brandon Ehle <az...@yahoo.com> writes:
> + /* Open-code parsing of the new timestamp format, as this
> + is a hot path for reading the entries file.
> + + This format looks like:
>
> + "2001-08-31T04:24:14.966996Z" */
> + exploded_time.tm_year = strtoul(c, (char**)&c, 10);
> + if (*c++ != '-') goto fail;
If I look at /usr/local/apache2/include/httpd.h it says
/** strtoul does not exist on sunos4. */
#ifdef strtoul
#undef strtoul
#endif
#define strtoul strtoul_is_not_a_portable_function_use_strtol_instead
Perhaps we should use strtol into a temporary, and check for -ve
values.
> + exploded_time.tm_mon = strtoul(c, (char**)&c, 10);
> + if (*c++ != '-') goto fail;
> + exploded_time.tm_mday = strtoul(c, (char**)&c, 10);
> + if (*c++ != 'T') goto fail;
> + exploded_time.tm_hour = strtoul(c, (char**)&c, 10);
> + if (*c++ != ':') goto fail;
> + exploded_time.tm_min = strtoul(c, (char**)&c, 10);
> + if (*c++ != ':') goto fail;
> + exploded_time.tm_sec = strtoul(c, (char**)&c, 10);
> + if (*c++ != '.') goto fail;
> + exploded_time.tm_usec = strtoul(c, (char**)&c, 10);
> + if (*c++ != 'Z') goto fail;
> +
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Brandon Ehle <az...@yahoo.com>.
>
>
>Ah. I remember now. I had hoped you'd repost the patch without the
>unnecessary reformatting. This seems to be it. Well, it's not entirely
>in the same format as the rest of the file, but upon looking at the code
>I see that svn_time_from_cstring is inconsistent already. "A foolish
>consistency is the hobgoblin of small minds."
>
>+1, I say it should go in.
>
>And, for the record, I propose we kill that
>space-before-parein-in-function-call rule.
>
>
According to the HACKING doc, this is optional.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2002-12-11 at 14:48, Michael Price wrote:
> "if (cond)" is nice, but "strcmp (...)" is annoying.
I think this can be the last word on the issue:
Right now we have a split among the most active developers (Karl likes
it, cmike likes it, brane and the Gregs dislike it, dunno about anyone
else), and consequentially, we have no rule except "be consistent within
a file, and don't change over a file once it's been created." In order
to have a rule, we would need to be much closer to consensus on the
issue. And we won't get consensus by arguing; we'll get everyone to be
Zoroastrian before we'll get everyone to agree on code style.
Obviously, if you care about this issue strongly, you should write as
much new code as possible so that you can make your preference apply to
lots of files. (That's the real reason I wrote ra_svn, of course.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Michael Price <mp...@atl.lmco.com>.
cmpilato@collab.net writes:
> > > And, for the record, I propose we kill that
> > > space-before-parein-in-function-call rule.
> >
> > +1.
> _
> -0.9999
>
> Sorry, but whitespace gives code a less cramped look, the same reason
> I prefer:
>
> if (cond)
> {
> stuff
> }
>
> to:
>
> if (cond) {
> stuff
> }
>
> But let's just not go here today, okay? Please?
"if" isn't a function call :)
"if (cond)" is nice, but "strcmp (...)" is annoying.
Personally, I put a space between all C/C++ keywords and the "(" but leave
the space out if its a function/method call.
Then again, I'm not a subversion developer so my comments don't carry that
much weight.
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Branko Čibej <br...@xbc.nu>.
cmpilato@collab.net wrote:
>Blair Zajac <bl...@orcaware.com> writes:
>
>
>
>>Branko ?ibej wrote:
>>
>>
>>>And, for the record, I propose we kill that
>>>space-before-parein-in-function-call rule.
>>>
>>>
>>+1.
>>
>>
> _
>-0.9999
>
>Sorry, but whitespace gives code a less cramped look, the same reason
>I prefer:
>
> if (cond)
> {
> stuff
> }
>
>to:
>
> if (cond) {
> stuff
> }
>
>But let's just not go here today, okay? Please?
>
I don't recall mentioning braces. Just parentheses, and only in function
calls:
abort()
instead of
abort ()
And I did _not_ propose to make sweeping formatting changes in the code,
just to mandate that new code drops that space-after-function-name thing
in function calls -- not in declarations, of course.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by cm...@collab.net.
Blair Zajac <bl...@orcaware.com> writes:
> Branko ?ibej wrote:
> >
> > And, for the record, I propose we kill that
> > space-before-parein-in-function-call rule.
>
> +1.
_
-0.9999
Sorry, but whitespace gives code a less cramped look, the same reason
I prefer:
if (cond)
{
stuff
}
to:
if (cond) {
stuff
}
But let's just not go here today, okay? Please?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Blair Zajac <bl...@orcaware.com>.
Branko ?ibej wrote:
>
> And, for the record, I propose we kill that
> space-before-parein-in-function-call rule.
+1.
Blair
--
Blair Zajac <bl...@orcaware.com>
Plots of your system's performance - http://www.orcaware.com/orca/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Branko Čibej <br...@xbc.nu>.
Brandon Ehle wrote:
> Branko Čibej wrote:
>
>> Isn't this a repost of your earlier patch? I thought we'd applied
>> that one.
>>
>>
> I believe it had been rejected because my editor went a little nuts
> with beautification.
Ah. I remember now. I had hoped you'd repost the patch without the
unnecessary reformatting. This seems to be it. Well, it's not entirely
in the same format as the rest of the file, but upon looking at the code
I see that svn_time_from_cstring is inconsistent already. "A foolish
consistency is the hobgoblin of small minds."
+1, I say it should go in.
And, for the record, I propose we kill that
space-before-parein-in-function-call rule.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Brandon Ehle <az...@yahoo.com>.
Branko Čibej wrote:
>Isn't this a repost of your earlier patch? I thought we'd applied that one.
>
>
I believe it had been rejected because my editor went a little nuts with
beautification.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn-fast-time-from-cstring.patch
Posted by Branko Čibej <br...@xbc.nu>.
Isn't this a repost of your earlier patch? I thought we'd applied that one.
Brandon Ehle wrote:
> * subversion/libsvn_subr/time.c(svn_time_from_cstring):
> Unroll the sscanf when converting time from a string for
> better performance.
>
> Index: subversion/libsvn_subr/time.c
> ===================================================================
> --- subversion/libsvn_subr/time.c (revision 3971)
> +++ subversion/libsvn_subr/time.c (working copy)
> @@ -142,44 +142,53 @@
> return -1;
> }
>
> -
> svn_error_t *
> svn_time_from_cstring(apr_time_t *when, const char *data, apr_pool_t
> *pool)
> {
> apr_time_exp_t exploded_time;
> apr_status_t apr_err;
> char wday[4], month[4];
> + const char *c = data;
>
> - /* First try the new timestamp format. */
> - if (sscanf (data,
> - timestamp_format,
> - &exploded_time.tm_year,
> - &exploded_time.tm_mon,
> - &exploded_time.tm_mday,
> - &exploded_time.tm_hour,
> - &exploded_time.tm_min,
> - &exploded_time.tm_sec,
> - &exploded_time.tm_usec) == 7)
> - {
> - exploded_time.tm_year -= 1900;
> - exploded_time.tm_mon -= 1;
> - exploded_time.tm_wday = 0;
> - exploded_time.tm_yday = 0;
> - exploded_time.tm_isdst = 0;
> - exploded_time.tm_gmtoff = 0;
> - - apr_err = apr_implode_gmt (when, &exploded_time);
> - if(apr_err != APR_SUCCESS)
> - {
> - return svn_error_createf (SVN_ERR_BAD_DATE, apr_err, NULL,
> - "Date conversion failed.");
> - }
> - - return SVN_NO_ERROR;
> - }
> - /* Then try the compatibility option. */
> - else if (sscanf (data,
> - old_timestamp_format,
> + /* Open-code parsing of the new timestamp format, as this
> + is a hot path for reading the entries file.
> + + This format looks like:
> + "2001-08-31T04:24:14.966996Z" */
> + exploded_time.tm_year = strtoul(c, (char**)&c, 10);
> + if (*c++ != '-') goto fail;
> + exploded_time.tm_mon = strtoul(c, (char**)&c, 10);
> + if (*c++ != '-') goto fail;
> + exploded_time.tm_mday = strtoul(c, (char**)&c, 10);
> + if (*c++ != 'T') goto fail;
> + exploded_time.tm_hour = strtoul(c, (char**)&c, 10);
> + if (*c++ != ':') goto fail;
> + exploded_time.tm_min = strtoul(c, (char**)&c, 10);
> + if (*c++ != ':') goto fail;
> + exploded_time.tm_sec = strtoul(c, (char**)&c, 10);
> + if (*c++ != '.') goto fail;
> + exploded_time.tm_usec = strtoul(c, (char**)&c, 10);
> + if (*c++ != 'Z') goto fail;
> +
> + exploded_time.tm_year -= 1900;
> + exploded_time.tm_mon -= 1;
> + exploded_time.tm_wday = 0;
> + exploded_time.tm_yday = 0;
> + exploded_time.tm_isdst = 0;
> + exploded_time.tm_gmtoff = 0;
> +
> + apr_err = apr_implode_gmt(when, &exploded_time);
> + if (apr_err == APR_SUCCESS)
> + return SVN_NO_ERROR;
> +
> + return svn_error_createf(SVN_ERR_BAD_DATE, apr_err, NULL,
> + "Date conversion failed.");
> +
> +fail:
> + /* Try the compatibility option. This does not need to be fast,
> + as this format is no longer generated and the client will convert
> + an old-format entries file the first time it reads it. */
> + if (sscanf(data, old_timestamp_format,
> wday,
> &exploded_time.tm_mday,
> month,
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org