Stop getting the `local_timezone` when it's not required

[irevoire]

Part of https://github.com/sharkdp/numbat/issues/525

The culprit

Hey, I profiled numbat once again while running 1 + 1 and noticed that 23% (~10ms out of 40) of the time was spent running the ffi::get_local_timezone function of numbat.

The call comes from this line: https://github.com/sharkdp/numbat/blob/84704073de791a8266653de9c09af80a29f21e16/numbat/modules/datetime/functions.nbt#L28-L29

And after commenting it and benchmarking the startup time with hyperfine here are the results I got:

% hyperfine './numbat-master -e "1 + 1"' './numbat-without-tz -e "1 + 1"' --warmup 10
Benchmark 1: ./numbat-master -e "1 + 1"
  Time (mean ± σ):      41.4 ms ±   1.0 ms    [User: 30.5 ms, System: 10.2 ms]
  Range (min … max):    39.9 ms …  45.0 ms    69 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 2: ./numbat-without-tz -e "1 + 1"
  Time (mean ± σ):      32.4 ms ±   1.6 ms    [User: 30.3 ms, System: 1.4 ms]
  Range (min … max):    30.6 ms …  36.1 ms    86 runs
 
Summary
  ./numbat-without-tz -e "1 + 1" ran
    1.28 ± 0.07 times faster than ./numbat-master -e "1 + 1"

So it’s pretty much what I measured with my profiler, 1.28 times or ~10ms faster.

The solution

I’m not sure of what should be the solution here though, maybe make the whole local variable a ffi function? Not a fan though, maybe you’ll have a better idea

[sharkdp]

Thank you! Not sure how we missed this in profiles so far. I guess it became slower with #511 because now we're loading the tz database from disk.

Conceptually, this is similar to the problem we had with currency conversion factors. We want to initialize variables/units with a value that is expensive to look up. Lazy loading would be an obvious solution here, but might be a bit tricky to implement? I haven't thought deeply about it but maybe, instead of calling get_local_timezone() at startup time, we could set

let local: Fn[(DateTime) -> DateTime] = _local()

where _local() would be a new FFI function that would immediately return lazy wrapper around the actual function reference (we have a special FunctionReference::TzConversion variant anyway…).

It would be fantastic if we could find a solution that would solve this uniformly, across currencies and timezone objects (and potentially others). Maybe some kind of language feature that allows variables and units to be defined lazily. Like a @lazy or @async decorator on that definition.

If all of that turns out to be completely over-engineered, I'd also be fine with turning local into a function :smile:

[irevoire]

If all of that turns out to be completely over-engineered, I'd also be fine with turning local into a function 😄

To me, this looks like the easiest for this specific case, but I’m not sure if it’s a breaking change. 🤔 And also, it means it'll be re-computed for every call instead of once for the whole lifetime of the program, I guess? Ideally, you’re right. A Lazy would be the best to accommodate both use cases. But shouldn’t it be the case for every variable by default instead of being a keyword?

[Goju-Ryu]

I have actually been thinking about how having lazy loaded values could be useful for function local variables. I have often needed to use an if-then-else expression to ensure that a calculation is valid because it is valid only in some branches. A lazy value would simplify this significantly in addition to the benefits for this and similar issues. I don't know if it is the best solution for this particular issue, but as a feature in general I think it would be beneficial in any case.

[sharkdp]

I have actually been thinking about how having lazy loaded values could be useful for function local variables. I have often needed to use an if-then-else expression to ensure that a calculation is valid because it is valid only in some branches.

Right. Looking into lazy evaluation in general sounds very appealing. This sounds like a bigger change though, so maybe we can find a (non breaking) workaround solution for this.