-
-
Notifications
You must be signed in to change notification settings - Fork 747
Optimized std.datetime.date.DateTime from string methods #5550
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
Conversation
|
Thanks for your pull request, @JackStouffer! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
2adb329 to
58ebf54
Compare
|
Speed results code import std.stdio;
import std.algorithm;
import std.conv;
import std.traits;
import std.datetime;
enum testCount = 5_000_000;
void main()
{
auto result = to!Duration(benchmark!(() => DateTime.fromISOString("20100704T070612"))(testCount)[0]);
writeln("result", "\t", result);
} |
| import std.string : strip; | ||
|
|
||
| immutable dstr = to!dstring(strip(isoString)); | ||
| immutable str = strip(isoString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work to use byCodeUnit here? Presumably, the speed-up is from getting rid of allocating the dstring, but you're introducing more auto-decoding in the process. If byCodeUnit doesn't work here at the moment (e.g. due to a function still requiring actual strings), then this is still clearly better, but I'd prefer to have byCodeUnit used if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmdavis None of the functions here use auto decoding. I've already removed it from Date.fromISOString and TimeOfDay.fromISOString, and countUntil with a single needle forwards to find, which already deals with auto-decoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that strip and countUntil both did, but if they don't, then that's not an issue (though actually, I should probably know for strip, since I did a rewrite of that at one point - but I guess that it was long enough ago that I forgot).
|
@jmdavis Thanks! |
In the vein of #5413 and #5404