speed up date time parsing #19545

shashi · 2016-12-09T10:49:17Z

This PR builds on @simonbyrne's #18000 and makes the (120x 😆) speed up available for custom date formats. It also re-does the DateFormat type to be sufficiently parameterized so that an efficient parsing function can be "generated". This also incidentally makes it easier to add new features to the parser.

before:

julia> @benchmark DateTime("2010-10-10T10:10:10.10")
BenchmarkTools.Trial: 
  memory estimate:  3.11 kb
  allocs estimate:  85
  --------------
  minimum time:     20.125 μs (0.00% GC)
  median time:      21.233 μs (0.00% GC)
  mean time:        21.818 μs (1.38% GC)
  maximum time:     3.128 ms (96.19% GC)
  --------------
  samples:          10000
  evals/sample:     1
  time tolerance:   5.00%
  memory tolerance: 1.00%

after:

julia> @benchmark DateTime("2010-10-10T10:10:10.10")
BenchmarkTools.Trial: 
  memory estimate:  0.00 bytes
  allocs estimate:  0
  --------------
  minimum time:     164.320 ns (0.00% GC)
  median time:      168.183 ns (0.00% GC)
  mean time:        168.666 ns (0.00% GC)
  maximum time:     291.389 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     750
  time tolerance:   5.00%
  memory tolerance: 1.00%

Also works fast for custom date format strings.

julia> @benchmark DateTime("01-01-2010X53:23:12", dateformat"dd-mm-YXS:M:H")
BenchmarkTools.Trial: 
  memory estimate:  0.00 bytes
  allocs estimate:  0
  --------------
  minimum time:     148.979 ns (0.00% GC)
  median time:      151.564 ns (0.00% GC)
  mean time:        153.853 ns (0.00% GC)
  maximum time:     327.571 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     818
  time tolerance:   5.00%
  memory tolerance: 1.00%

fixes #15888, #13644
supersedes #18000

tkelman · 2016-12-09T11:39:15Z

base/dates/io.jl


 """
    DateTime(dt::AbstractString, df::DateFormat) -> DateTime

 Construct a `DateTime` by parsing the `dt` date string following the pattern given in
-the [`DateFormat`](@ref) object. Similar to
+the [`DateFormat`](:func:`Dates.DateFormat`) object. Similar to


bad rebase here

tkelman · 2016-12-09T11:39:52Z

base/exports.jl

@@ -1009,6 +1009,8 @@ export
 # dates
    Date,
    DateTime,
+    DateFormat,
+    @dateformat_str,


should be added to the manual index if exported

How do I do this?

see contributing.md

If you meant I need to add this to the manual, I have. I was wondering if there's some kind of list I need to update when you said "manual index"

not talking about doc/manual, talking about doc/stdlib for the docstring. yes there is a list, again see contributing.md

Ah, okay. Will do.

StefanKarpinski · 2016-12-09T19:01:42Z

Amazing work, @shashi! I would note that this still doesn't preclude having even more efficient custom code for the three or so common date formats.

shashi · 2016-12-10T04:50:59Z

@StefanKarpinski that's right, and you don't even have to give them a name and add them to a type hierarchy. :) Just dispatch on typeof(dateformat"mycommonformat").

ViralBShah · 2016-12-10T04:58:42Z

Can this be backported to 0.5?

shashi · 2016-12-10T06:44:38Z

It breaks compatibility with the previous DateFormat type (presumably DateFormat was an implementation detail) and changes parsing behavior in some corner cases (see diff of tests)...

tkelman · 2016-12-10T22:39:21Z

test/dates/io.jl

@@ -268,7 +275,7 @@ f = "y m d"
 @test_throws ArgumentError Dates.Date(" 1 1 32",f)
 @test_throws ArgumentError Dates.Date("# 1 1 32",f)
 # can't find 1st space delimiter,s o fails


comment no longer applies?

omus · 2016-12-11T00:14:07Z

base/dates/types.jl

+    rata = xs[7] + 1000*(xs[6] + 60*xs[5] + 3600*xs[4] + 86400*Base.Dates.totaldays(xs[1],xs[2],xs[3]))
+    return DateTime(Base.Dates.UTM(rata))
+end
+


What's the advantage of having this duplicate constructor versus:

DateTime(t::NTuple{7,Int}) = DateTime(t[1], t[2], t[3], t[4], t[5], t[6], t[7])

Current implementation will run into issues with integer overflow on 32-bit systems

There was a cost to splatting so I added this, forgot to remove / define one with the other. Was going to do this, thanks!

The tests are failing on 32 bit machines because of overflow. I modified this method to convert the input tuple to Int32s but wasn't able to reproduce the failure. I'm wondering how tests passed in the previous implementation if the constructor is the problem. Do you have an idea of where exactly it breaks?

Or just DateTime(t...)

omus · 2016-12-11T00:14:58Z

base/dates/io-util.jl

+        return R(reorder_args(parts, fmt.field_order, fmt.field_defaults, err_idx)::NTuple{7,Int})
+
+        @label error
+        return R((err_idx,state,0,0,0,0,0), true)


If you want to indicate null here you want false

fixed, added a test

Yes, this was a 0.6 change, so may give different results than 0.5

simonbyrne · 2016-12-12T15:00:45Z

base/dates/io-util.jl

+    end
+end
+
+_create_timeobj(tup, T::Type{DateTime}) = T(tup)


why not T(tup...)?

I fixed changed this in a later commit. this review business is a bit confusing. Thanks for reviewing!

simonbyrne · 2016-12-12T15:04:53Z

This looks pretty awesome. A few minor changes mostly pointed out by @omus.

shashi · 2016-12-09T11:01:26Z

base/dates/io-util.jl

+        @label error
+        return R((err_idx,state,0,0,0,0,0), true)
+    end
+end


@vtjnash do you know of a way to make this not generated, but still get the same performance?

I know the classic way of doing it in a static language (hardcoding a jump table / switch / vtable), but afaik there isn't really a good way to express that optimization in Julia right now (other than by doing a generated function like this). fwiw, since this only uses @nexpr, it appears to be a completely valid generated function.

shashi · 2016-12-10T03:18:23Z

base/exports.jl

@@ -1009,6 +1009,8 @@ export
 # dates
    Date,
    DateTime,
+    DateFormat,
+    @dateformat_str,


How do I do this?

shashi · 2016-12-11T02:04:49Z

base/dates/types.jl

+    rata = xs[7] + 1000*(xs[6] + 60*xs[5] + 3600*xs[4] + 86400*Base.Dates.totaldays(xs[1],xs[2],xs[3]))
+    return DateTime(Base.Dates.UTM(rata))
+end
+


There was a cost to splatting so I added this, forgot to remove / define one with the other. Was going to do this, thanks!

The tests are failing on 32 bit machines because of overflow. I modified this method to convert the input tuple to Int32s but wasn't able to reproduce the failure. I'm wondering how tests passed in the previous implementation if the constructor is the problem. Do you have an idea of where exactly it breaks?

shashi · 2016-12-12T08:35:26Z

base/dates/io-util.jl

+        return R(reorder_args(parts, fmt.field_order, fmt.field_defaults, err_idx)::NTuple{7,Int})
+
+        @label error
+        return R((err_idx,state,0,0,0,0,0), true)


fixed, added a test

shashi · 2016-12-12T16:00:28Z

base/dates/io-util.jl

+    end
+end
+
+_create_timeobj(tup, T::Type{DateTime}) = T(tup)


I fixed changed this in a later commit. this review business is a bit confusing. Thanks for reviewing!

omus · 2016-12-12T17:38:10Z

I am concerned about these changes as they break support for custom parser slots which don't return integers. Specifically, these new changes won't work with parsing TimeZone types.

vtjnash · 2016-12-12T18:47:34Z

base/dates/io.jl

+is used. This object is passed as the last argument to
+`tryparsenext` and `format` defined for each `AbstractDateToken` type.
+"""
+immutable DateLocale{lang} end


I strongly recommend against baking lang into a type parameter. This will actually make the API more awkward to extend than presenting it to the user as a dict.

Okay. I can make this type hold two dictionaries.

We may want to think about how this can be generalised to other locale-based print functions (e.g. decimal separators), though that may be better in a different PR.

vtjnash · 2016-12-12T18:56:23Z

base/dates/io-util.jl

+        @label error
+        return R((err_idx,state,0,0,0,0,0), true)
+    end
+end


I know the classic way of doing it in a static language (hardcoding a jump table / switch / vtable), but afaik there isn't really a good way to express that optimization in Julia right now (other than by doing a generated function like this). fwiw, since this only uses @nexpr, it appears to be a completely valid generated function.

simonbyrne · 2016-12-14T17:10:51Z

@nanosoldier runbenchmarks("dates", vs = ":master")

nanosoldier · 2016-12-14T18:59:29Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

davidanthoff · 2016-12-14T20:22:46Z

Might it make sense to do #16928 as part of this as well?

omus · 2016-12-14T21:54:28Z

I'm refactoring @shashi's code to clean it up and maybe find some additional performance tweaks (I've found a couple). I'll be update #19519 with this work unless someone feels I should open a new PR and close #19519.

ViralBShah · 2016-12-14T22:07:20Z

I don't see any reason to open a new PR.

simonbyrne · 2016-12-14T22:29:40Z

Interesting: I wonder why the printing is now slower?

shashi · 2016-12-15T03:40:54Z

@simonbyrne I did not benchmark printing speeds, but had to redo it to accomodate the new DateFormat. There should be easy speedups available.

JeffBezanson · 2016-12-15T05:09:30Z

This is some excellent work. Real julia wizardry on display here!

shashi · 2016-12-15T07:08:19Z

Thanks @JeffBezanson 😄

shashi · 2016-12-15T08:29:40Z

@omus I have merged your changes in here with the required conflict resolutions.

Do pull them, and let me know when you have more updates so that I can pull them into this PR.

nanosoldier · 2017-01-19T03:43:10Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

tkelman · 2017-01-19T17:23:23Z

The regressions are real, but we should get this in before feature freeze. @omus @shashi have you looked into fixing them?

shashi · 2017-01-20T15:42:32Z

I tried reverting the change. On my machine there's barely any change. I don't know how #19545 (comment) gave a green tick for these benchmarks.

tkelman · 2017-01-20T22:20:15Z

I'm not sure, but it's possible this might be hitting problems related to #20025 ?

tkelman · 2017-01-20T22:24:32Z

That good set of results was just before we merged the llvm 3.9 upgrade, so newer llvm changes the performance characteristics of the patches here w.r.t. those benchmarks.

StefanKarpinski · 2017-01-21T02:59:32Z

Wait, so what's the status here? The string change is so kickass that it makes these speed ups irrelevant?

shashi · 2017-01-21T03:01:02Z

@StefanKarpinski No, there's a regression in converting DateTime to string which I'm trying to track down. Parsing is still much much faster with this.

shashi · 2017-01-23T12:28:31Z

base/dates/io.jl

+        dateformat"YYYY-mm-dd\THH:MM:SS"
+    else
+        dateformat"YYYY-mm-dd\THH:MM:SS.s"
+    end


@omus @quinnj is it okay if we always use "YYYY-mm-dd\THH:MM:SS.s" here? This type instability adds to the printing slow down. Users wanting to ignore milliseconds can specify dateformat"YYYY-mm-dd\THH:MM:SS"

Fine by me.

Oh, wait. this is not really a problem...

Glad it's not a problem. If we were to change this I think I would prefer "YYYY-mm-dd\THH:MM:SS.sss"

shashi · 2017-01-23T14:14:35Z

@nanosoldier runbenchmarks("dates", vs = ":master")

nanosoldier · 2017-01-23T16:15:22Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

quinnj · 2017-01-23T16:24:57Z

DateTime printing looks good! Do we just need to apply the same changes to Date?

omus · 2017-01-23T18:04:44Z

base/dates/io.jl

+        write(io, str)
+    else
+        l = endof(str)
+        write(io, SubString(str,l-(n-1),l))
    end


Should probably save endof(str) before the if.

tkelman · 2017-01-24T05:16:08Z

base/dates/io.jl

+    if millisecond(dt) == 0
+        format(dt, dateformat"YYYY-mm-dd\THH:MM:SS", 24)
+    else
+        format(dt, dateformat"YYYY-mm-dd\THH:MM:SS.s", 26)


are milliseconds in a DateTime only ever a single digit?

No, s stands for any number of digits e.g. s gives .5 or .05 or .005 while .sss would give .500, .050.

so is the 26 a minimum?

No it's usually 23 chars, I added 3 more bytes to keep this going fast even when the year is -10000 ;) I'm not sure if this really matters, doesn't julia allocate the buffer backing an array in multiples of 16 bytes or something?

Not sure. As long as this is just a rough-guess buffer preallocation and things work fine if the actual output is longer or shorter then I'll just go with whatever the benchmarks say.

shashi · 2017-01-24T06:31:35Z

Date formatting (to arbitrary formats) is 5x faster than before. So in a bit of a cop out, I changed string(::Date) to be the same as before. I guess the speed up mainly comes from not allocating an IOBuffer object.

@nanosoldier runbenchmarks("dates", vs = ":master")

tkelman · 2017-01-24T06:35:44Z

base/dates/io.jl

+    end
+end
+
+function Base.string(dt::Date)


do you want to delete the now-duplicate one at line 46 then?

Oh, yes, I should overlooked that when resolving merge conflicts.

…g_merge

nanosoldier · 2017-01-24T08:32:35Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

tkelman · 2017-01-24T13:08:43Z

base/dates/io.jl

+format the `tok` token from `dt` and write it to `io`. The formatting can
+be based on `locale`.
+
+all subtypes of `AbstractDateToken` must define this method in order


First word in a sentence should be capitalized

Performance improvements introduced in Base.Dates no longer allow for TimeZones to extend the functionality. JuliaLang/julia#19545

shashi requested review from nalimilan and quinnj December 9, 2016 10:50

shashi added dates Dates, times, and the Dates stdlib module performance Must go faster labels Dec 9, 2016

shashi requested a review from simonbyrne December 9, 2016 10:51

tkelman reviewed Dec 9, 2016

View reviewed changes

tkelman reviewed Dec 10, 2016

View reviewed changes

omus reviewed Dec 11, 2016

View reviewed changes

simonbyrne reviewed Dec 12, 2016

View reviewed changes

shashi commented Dec 12, 2016

View reviewed changes

vtjnash reviewed Dec 12, 2016

View reviewed changes

omus mentioned this pull request Dec 14, 2016

WIP: Further improving fast date parsing #19519

Merged

tkelman added this to the 0.6.0 milestone Jan 19, 2017

shashi commented Jan 23, 2017

View reviewed changes

omus reviewed Jan 23, 2017

View reviewed changes

tkelman reviewed Jan 24, 2017

View reviewed changes

[ci skip] further speed up date time printing

ddf2be6

shashi force-pushed the sh/fast_date_parsing branch from 51a7cbc to cbe5577 Compare January 24, 2017 06:26

tkelman reviewed Jan 24, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into sh/fast_date_parsin…

2a86b75

…g_merge

shashi force-pushed the sh/fast_date_parsing branch from cbe5577 to 2a86b75 Compare January 24, 2017 08:00

shashi merged commit 947347f into master Jan 24, 2017

tkelman reviewed Jan 24, 2017

View reviewed changes

tkelman deleted the sh/fast_date_parsing branch January 24, 2017 13:21

omus mentioned this pull request Feb 21, 2017

io.slotparse, io.slotformat fail on v0.6-dev JuliaTime/TimeZones.jl#44

Closed

This was referenced Mar 3, 2017

Dates.parse(::AbstractString, ::DateFormat) removed without deprecation #20876

Closed

Extensible and fast date parsing #20952

Merged

omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Mar 16, 2017

Temporarily restrict parsing pre-Julia 0.6

a665730

Performance improvements introduced in Base.Dates no longer allow for TimeZones to extend the functionality. JuliaLang/julia#19545

omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Mar 16, 2017

Temporarily restrict parsing pre-Julia 0.6

9d999c6

Performance improvements introduced in Base.Dates no longer allow for TimeZones to extend the functionality. JuliaLang/julia#19545

omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Mar 16, 2017

Temporarily restrict parsing pre-Julia 0.6

5c399e4

Performance improvements introduced in Base.Dates no longer allow for TimeZones to extend the functionality. JuliaLang/julia#19545

speed up date time parsing #19545

speed up date time parsing #19545

Conversation

shashi commented Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented Dec 9, 2016

shashi commented Dec 10, 2016 • edited Loading

ViralBShah commented Dec 10, 2016

shashi commented Dec 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbyrne commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbyrne commented Dec 14, 2016

nanosoldier commented Dec 14, 2016

davidanthoff commented Dec 14, 2016

omus commented Dec 14, 2016

ViralBShah commented Dec 14, 2016

simonbyrne commented Dec 14, 2016

shashi commented Dec 15, 2016

JeffBezanson commented Dec 15, 2016

shashi commented Dec 15, 2016

shashi commented Dec 15, 2016 • edited Loading

nanosoldier commented Jan 19, 2017

tkelman commented Jan 19, 2017

shashi commented Jan 20, 2017

tkelman commented Jan 20, 2017

tkelman commented Jan 20, 2017

StefanKarpinski commented Jan 21, 2017

shashi commented Jan 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashi commented Jan 23, 2017

nanosoldier commented Jan 23, 2017

quinnj commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashi commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented Jan 24, 2017

Choose a reason for hiding this comment

shashi commented Dec 9, 2016 •

edited

Loading

shashi commented Dec 10, 2016 •

edited

Loading

shashi commented Dec 15, 2016 •

edited

Loading