-
-
Notifications
You must be signed in to change notification settings - Fork 746
Added fast paths in std.utf.byUTF #4642
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
|
please benchmarks with combination with previous one PR |
|
Just tested it, still slower and with no noticeable difference in speed. Hmm, I'm looking at the profile output and I don't see what could be causing the 8x slower speeds. |
|
maybe inlining bug? |
|
I don't think so, but I am a novice at assembly. I found LDC's output of the function, but I don't see a call for it anywhere, so I guess it was inlined at every call point |
|
You can also try the LLVM IR output ( |
std/utf.d
Outdated
| buf, decodeFront!(UseReplacementDchar.yes)(r)); | ||
| auto c = r.front; | ||
|
|
||
| if (c.isASCII) |
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.
Possibly isASCII is not inlined, just use plain old < 0x80
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.
No effect
|
Ok, I think I've figured it out. Will post numbers soon. |
|
Well, that did make the UTF string faster, but I forgot that the ascii string isn't using that code path, so no change :/ |
|
Regardless of the other PR, these are two good optimizations that IMO should be pulled. |
095939f to
47f0c1d
Compare
|
Fixed test errors |
Current coverage is 88.68% (diff: 100%)@@ master #4642 diff @@
==========================================
Files 121 121
Lines 73827 73833 +6
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 65471 65477 +6
Misses 8356 8356
Partials 0 0
|
|
Ping |
|
Ping @9il |
How about starting to include such benchmarks in a special I believe @burner is envisioning sth. similar with #2995, but we could still start to collect the benchmark script as (a) it comes "for free" (they are already written) and (b) might help to move forward with a future "full-blown" benchmark suite as we then already have real scripts and know what functions / features are lacking. Btw runtime has already gone a similar way. (pinging @qznc as he has written a nice blog post about benchmarking Phobos) |
|
Ping @9il @klickverbot I think these two optimizations are good to go regardless of the other PR. |
|
@andralex Thanks! |
@9il @DmitryOlshansky