-
Notifications
You must be signed in to change notification settings - Fork 533
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
Split clock
feature into clock-now
and clock-tz
#1341
Comments
Sounds reasonable, want to send a PR to the 0.4.x branch? Can you link to issues with more details on the perf impact you've seen? |
@djc Totally, we can PR that. I'll also ping some folks on our team to pull out more concrete data. We have been running experiments for the past year on startup costs of various portions of Deno and one bit that stood out was the cost of loading various frameworks on Mac -- CoreFoundation and Security in particular. The former is pulled in via |
I built a very simple example that roughly corresponds to the additional overhead we are seeing in Deno. While the absolute number is quite small, it's about 5-10% of the work we need to hit our startup time budgets: [package]
name = "chrono-test"
version = "0.1.0"
edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[dependencies]
chrono = { git = "https://github.com/mmastrac/chrono", branch = "split_clock_feature", default-features = false, features = ["clock"] } use chrono::Utc;
fn main() {
println!("Hello, world! {}", Utc::now());
}
11:09 $ otool -L with-cf
with-cf:
/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1971.0.0)
/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3)
11:12 $ otool -L no-cf
no-cf:
/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3) Benchmarking on my local machine shows a ~0.3 ms difference (approx) in overhead from
|
Okay, and have you figured out what "the work" is that is impacting performance? I wonder if there is something here that we (in chrono or our dependencies) should be doing more lazily so you're not impacted at all. |
At least on Mac, I suspect we would need to upstream changes to the iana-time-zone crate to open the CoreFoundation framework on first use. It's likely we'll do that as well, though it's a more complex patch. The extra time is unfortunately just what the OSX loader appears to require to load the framework. It doesn't require using a TZ -- just linking the framework is enough to pay that penalty. I suspect (most) other platforms don't pull in link-time libraries for the current TZ so it's very likely a Mac-only issue. |
Hey, the time is spent by |
Can this issue be closed now that the |
The
clock
feature currently pulls in timezone features which pulls in a bunch of crates to deal with timezones (iana-time-zone
,android-tz
). Some of these crates pull in system libraries that negatively affect startup time for Deno, which means we need to create an inconvenient shim for getting the current UTC time: https://github.com/denoland/deno/blob/main/cli/main.rs.It would be great if the feature could be split into smaller sub-features: one for reading the current time, and one for reading tz data, which would allow us to use
Utc::now()
without requiring timezone data.The text was updated successfully, but these errors were encountered: