Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TimeZones build performance regression #17072

Closed
iamed2 opened this issue Jun 23, 2016 · 3 comments
Closed

TimeZones build performance regression #17072

iamed2 opened this issue Jun 23, 2016 · 3 comments
Labels
performance Must go faster regression Regression in behavior compared to a previous version

Comments

@iamed2
Copy link
Contributor

iamed2 commented Jun 23, 2016

On 0.4:

               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.4.7-pre+1 (2016-06-19 17:17 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 57d0834* (3 days old release-0.4)
|__/                   |  x86_64-apple-darwin15.5.0

julia> include("/Users/ericdavies/.julia/v0.4/Timezones/deps/build.jl")
INFO: Downloading latest tz database
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  306k  100  306k    0     0   125k      0  0:00:02  0:00:02 --:--:--  126k
INFO: Extracting tz database archive
x africa
x antarctica
x asia
x australasia
x europe
x northamerica
x southamerica
INFO: Converting tz database into TimeZone data
INFO: Successfully processed TimeZone data

julia> @time compile(TZDATA_DIR, COMPILED_DIR)
 10.541140 seconds (2.66 M allocations: 132.962 MB, 0.44% gc time)

On 0.5:

               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.5.0-dev+4850 (2016-06-18 03:25 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 9ae4520* (5 days old master)
|__/                   |  x86_64-apple-darwin15.5.0

julia> include("/Users/ericdavies/.julia/v0.5/Timezones/deps/build.jl")
INFO: Downloading latest tz database
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  306k  100  306k    0     0   120k      0  0:00:02  0:00:02 --:--:--  120k
INFO: Extracting tz database archive
x africa
x antarctica
x asia
x australasia
x europe
x northamerica
x southamerica
INFO: Converting tz database into TimeZone data
INFO: Successfully processed TimeZone data

julia> @time compile(TZDATA_DIR, COMPILED_DIR)
210.887527 seconds (10.10 M allocations: 496.789 MB, 0.21% gc time)

Unfortunately I cannot seem to run a @profile (Julia hangs and can't be SIGINTd) in either version of Julia on this call. I seem to remember the regression being related to codegen/inference when running profiles previously.

This is the same version of the code (master) but this has been the case for previous versions.

@JeffBezanson JeffBezanson added performance Must go faster regression Regression in behavior compared to a previous version labels Jun 24, 2016
@JeffBezanson
Copy link
Member

I think this was fixed by #17052. In 0.5 you need to use x->!f(x) instead of @eval x->!($f)(x). This now takes 15 seconds. However, I noticed that the code in timezones/Olson.jl has the same issue, so I applied the following patch:

index 68af73e..97ee976 100644
--- a/src/timezones/Olson.jl
+++ b/src/timezones/Olson.jl
@@ -166,17 +166,17 @@ function ruleparse(from, to, rule_type, month, on, at, sav
         # The first day of the week that occurs before or after a given day of 
         # i.e. Sun>=8 refers to the Sunday after the 8th of the month
         # or in other words, the 2nd Sunday.
-        dow = DAYS[match(r"\w\w\w", on).match]
-        dom = parse(Int, match(r"\d\d?", on).match)
+        dow::Int = DAYS[match(r"\w\w\w", on).match]
+        dom::Int = parse(Int, match(r"\d\d?", on).match)
         if ismatch(r"<=", on)
-            on_func = @eval (dt -> day(dt) <= $dom && dayofweek(dt) == $dow)
+            on_func = (dt -> day(dt) <= dom && dayofweek(dt) == dow)
         else
-            on_func = @eval (dt -> day(dt) >= $dom && dayofweek(dt) == $dow)
+            on_func = (dt -> day(dt) >= dom && dayofweek(dt) == dow)
         end
     elseif ismatch(r"\d\d?", on)
         # Matches just a plain old day of the month
         dom = parse(Int, on)
-        on_func = @eval (dt -> day(dt) == $dom)
+        on_func = dt -> day(dt) == dom
     else
         error("Can't parse day of month for DST change")
     end

After that, the time is --- wait for it --- 0.756 seconds.

@vtjnash
Copy link
Member

vtjnash commented Jun 24, 2016

Ah, excellent. The good part about working on fixing #265 (e.g. #17057) was that this old definition will no longer work at all (which is how I discovered the one in Base.Dates).

@iamed2
Copy link
Contributor Author

iamed2 commented Jun 24, 2016

Excellent! It's definitely fixed on master. And good catch on the evals! I'm always on the lookout but I forgot to hunt for evals in TimeZones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

3 participants