Consider switching from chrono to jiff
#7852
Comments
|
Prototype in 133b7cb, updating
CI in progress here: https://github.com/drinkcat/coreutils/actions?query=branch%3Ajiff-dirty |
|
Aye. For |
|
Converted Size is "okay": Performance loss isn't good (14%), there's some minor optimization I can do (not call |
|
I don't think scanning the format string is the long pole in the tent here. I don't think Jiff's |
|
Maybe! For reference, |
|
So I've been trying to reproduce your benchmark, but haven't had much luck: I've tried a few different directories, but your branch seems consistently faster than If I look at a profile (on a bigger directory, my checkout of the Linux kernel), it looks like Jiff is a very small percentage of time here?
So I'm curious if perhaps I am missing a component of the benchmark here. I built |
This actually never got any optimization attention, and it looks like its perf matters to coreutils. So let's take a look at it! Ref uutils/coreutils#7852
This actually never got any optimization attention, and it looks like its perf matters to coreutils. So let's take a look at it! Ref uutils/coreutils#7852
|
This PR brought in some perf improvements to Jiff's |
|
One thing that also sticks out to me, is that |
|
First, thanks for being so responsive! This is awesome ,-)
Are you sure that you are using the latest
I like https://share.firefox.dev/4jziUdL
jiff@f21740ee5fc577c8cf4c2cab18f4049124203c3e doesn't seem to help at all ,-(
Yes there are still optimizations to be chased there, there are likely many unneeded string copies (some may be needed for alignment, some probably not, we could audit). So, yes, this saves maybe 1% performance (but it's difficult to measure): |
|
OK, it looks like I was using Given that As for profiling, doing it in debug mode without optimizations enabled is not meaningful. Like, that profile you shared is totally bunk for measuring real world performance because there's no optimization happening. e.g., Almost no inlining. The number of samples there is also super low. I too also use samply. But it and Let me try that. So from your fork, here's how I built Then I did the same for In order to make it possible to fully reproduce this, I decided to run And here's the actual benchmark. I also added Now let's profile. And indeed, in my profile above, I was redirecting output to What I see is that almost no time is spent in formatting:
I also see that the vast majority of time is being spent in directory traversal. As the author of
Now, I also ran this under samply, including redirecting to Then I did the same, but with Jiff (using Looking at the flamegraphs, it seems like with Jiff, formatting has almost disappeared from the profile. And even then, and similar to Chrono, it looks like a big chunk of the time is just spent in allocating the So I took a closer look at the actual code, and it looks really easy to avoid this intermediate alloc. (Your approach doesn't quite do it, because your And then re-running the benchmark: Which gives a slight improvement. This IMO pretty firmly solidifies that |
|
Interesting thanks. Yeah I just added the I wouldn't call what you did "easy", couldn't find the right API to deal with the Benchmarks are interesting... I also see extremely little difference on (an idea of the tree sizes: ) Anyway, thanks again, I'll integrate your changes, so that we're on par performance wise ,-) And I'll keep looking at what it'll take to convert more of the chrono usage. |
Oh that might work too. I avoided that because it avoids going through I'm eager to unblock y'all on switching to Jiff. I plan to get the remaining |
|
Oh and sorry, by "easy," I meant that your code was structured in a way where the intermediate alloc wasn't load bearing. I definitely grant that discovering the right APIs in Jiff may not be easy, especially the lower level ones. When it comes to parsing and formatting, Jiff is more like an onion. There's the nice and convenient APIs on the datetime/duration types directly, and then there's the more flexible but less convenient APIs inside of |
From code provided in uutils#7852 by @BurntSushi. Depending on the benchmarks, there is _still_ a small performance difference (~4%) vs main, but it's seen mostly on small trees getting printed repeatedly, which is probably not a terribly interesting use case.
|
RE lenient One thing worth pointing out here is that, as far as I can tell, POSIX itself doesn't seem to require this sort of lenient parsing. My understanding is that uutils is trying to port GNU coreutils and not POSIX coreutils, which I think means any sub-optimal user experience that GNU offers that could in theory be fixed within the boundary of POSIX compatibility isn't something uutils will do. I think this is somewhat unfortunate, but I get it. |
|
@drinkcat Do you want to give current master a whirl and see how it works for you? You should now have use jiff::{civil, fmt::strtime::{BrokenDownTime, Config}};
let tm = BrokenDownTime::from(civil::date(2025, 4, 30));
assert_eq!(
tm.to_string("%F %z").unwrap_err().to_string(),
"strftime formatting failed: %z failed: \
requires offset to format time zone offset",
);
// Now enable lenient mode:
let config = Config::new().lenient(true);
assert_eq!(
tm.to_string_with_config(&config, "%F %z").unwrap(),
"2025-04-30 %z",
);
// Lenient mode also applies when using an unsupported
// or unrecognized conversion specifier. This would
// normally return an error for example:
assert_eq!(
tm.to_string_with_config(&config, "%+ %0").unwrap(),
"%+ %0",
); |
From code provided in uutils#7852 by @BurntSushi. Depending on the benchmarks, there is _still_ a small performance difference (~4%) vs main, but it's seen mostly on small trees getting printed repeatedly, which is probably not a terribly interesting use case.
|
@BurntSushi awesome thanks! Looks like lenient code and the new formats work! I just updated my branch so we'll see what CI says, but local testing is good at least ,-) Edit: CI passes too! |
Based on discussion in #7849, it seems like it would be interesting to use
jiffinstead ofchrono, at the very least indate: this would make our life easier when handling timezones.Dirty branch here: https://github.com/drinkcat/coreutils/tree/jiff-dirty .
I'll keep modifying this comment as I find issues.
%q(quarters)%N(nanoseconds, forchronowe manually replaced this with%f)%X(localization issue, can be replaced manually with%H:%M:%Sfor now, I think localization is also TBD incoreutils)%:::z(see Some input indateare panicking the binary #3780)ls, we need to print dates using the same format over and over again.chronoprovides an optimized API for that use case: we parse the format string once, then use that to print many times (see fc6b896). I didn't do any benchmarking yet, but I suspect this might be an issue.@BurntSushi FYI
The text was updated successfully, but these errors were encountered: