1068014 - skip strptime() in datetime_from() if the date is in a standard format

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[:glob ✱]

Bugzilla::Util::datetime_from() always calls strptime to parse the date.

strptime is a generic date string parser, however most of the dates that we parse are strings that have been read from the database and are in a known format.

date parsing is about 8 times quicker if we use a regex for the known formats first, and fall back to strptime if it doesn't match.

Comment 1

[:glob ✱]

Attachment: 1068014_1.patch

Comment 2

[Frédéric Buclin]

Comment on attachment 8490073 [details] [diff] [review]
1068014_1.patch

>+    if ($date =~ /^(\d\d\d\d)[\.-](\d\d)[\.-](\d\d) (\d\d):(\d\d):(\d\d)$/) {

Could you improve your regexp to also accept dates in the YYYY-MM-DD format, i.e. without a time specified? Your regexp would become

  /^(\d\d\d\d)[\.-](\d\d)[\.-](\d\d)(?: (\d\d):(\d\d):(\d\d))?$/

and would behave exactly the same way as strptime().

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8490073 [details] [diff] [review]
1068014_1.patch

Review of attachment 8490073 [details] [diff] [review]:
-----------------------------------------------------------------

I think the regexp only matching date + time is acceptable with the justification that full datetime strings are much more common, and as an optimization we want to target the common case.

::: Bugzilla/Util.pm
@@ +554,5 @@
>  
> +    my @time;
> +    # Most dates will be in this format, avoid strptime's generic parser
> +    if ($date =~ /^(\d\d\d\d)[\.-](\d\d)[\.-](\d\d) (\d\d):(\d\d):(\d\d)$/) {
> +        @time = ($6, $5, $4, $3, $2 - 1, $1 - 1900, undef);

I would consider using \d{4} and \d{2}, which allow the regex engine to perform the match in fewer operations.

Given correct input, /^(\d{4})[\.-](\d{2})[\.-](\d{2}) (\d{2}):(\d{2}):(\d{2})$/ will match in just 39 steps,
but /^(\d\d\d\d)[\.-](\d\d)[\.-](\d\d) (\d\d):(\d\d):(\d\d)$/ will take 55 steps.
For non-matching strings, the difference is greater.

Comment 4

[Dylan Hardison [:dylan] (he/him)]

Is it still acceptable to set the milestone for this to 5.0?

Comment 5

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #3)
> I think the regexp only matching date + time is acceptable with the
> justification that full datetime strings are much more common

My suggestion is so trivial to implement that it would be a shame to not do it. We have date-only fields (deadline + custom fields) which call datetime_from(), see bug 1046168, and so you get some perf win here too.

Comment 6

[:glob ✱]

Attachment: 1068014_2.patch

given how easy it is, i think it's worth supporting timeless dates.

here's a revised patch which incorporates that, as well as the regex repetition change.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8490816 [details] [diff] [review]
1068014_2.patch

Review of attachment 8490816 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

r=dylan

Comment 8

[:glob ✱]

To ssh://[e-mail]/bugzilla/bugzilla.git
   1bead98..af46080  master -> master

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   c8c2d8e..4c16c87  master -> master