-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add :fast_ascii
mode to String.valid?/2
#12360
Conversation
Hi @mtrudel! These optimizations look quite promising 🤩 I was thinking of an alternative where we wouldn't need to introduce an extra # optimistic loop, able to process big ASCII-only binaries very fast
def valid?(<<a::56, rest::bits>>) when Bitwise.band(0x80808080808080, a) == 0 do
valid?(rest)
end
# slower loop for other cases
def valid?(other) when is_binary(other), do: valid_non_only_ascii?(other)
defp valid_non_only_ascii?(<<_::utf8, rest::bits>>), do: valid_non_only_ascii?(rest)
defp valid_non_only_ascii?(<<>>), do: true
defp valid_non_only_ascii?(_), do: false These early benchmarks look promising, especially with inlining, but I didn't check with various inputs and haven't installed OTP26. WDYT? |
That's a really interesting idea! I like it, but there are three aspects of it that may preclude it:
Happy to hew either way here, depending on how others feel! |
@mtrudel thank you for the detailed answers, these are all great points.
Same for me, I don't have any strong opinion, just wanted to share the idea. |
I think this patch is good to go, thank you! One last concern is: if the Erlang/OTP team decide to accept an optimized UTF-8 validation (from the simdutf8 library), then this patch may be pointless. According to the paper, the simdutf8 is several times faster than the ascii check implemented in C. So my suggestion is to keep this around until Erlang/OTP 26 is out. :) |
Just dropping my 2 cents regarding the naming of the argument |
I'd taken the naming from |
Agreed! The absolute best approach here would be to have a
Sure! Their release milestone doesn't mention anything about it, but seeing as this work is of no benefit on earlier versions, there's no real rush. Whatever you think is easiest! |
Oh, I thought you were planning to submit a PR with |
I very much do plan to submit such a PR, but looking at all the things I'm trying to get done for ElixirConf EU (big news on the Bandit front!), (plus my real job 😄) means I'm not going to be able to do it on that timeline. I'm hoping, roughly, to get this done May-ish. |
lib/elixir/lib/string.ex
Outdated
|
||
Note that the `:fast_ascii` algorithm does not affect correctness, you can expect the output of | ||
`String.valid?/2` to be the same regardless of algorithm. The only difference to be expected is | ||
one of performance, which can be expected to improve roughly quadratically in string length |
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.
improve roughly quadratically
I'm struggling a bit to understand this one, given that both algorithms are linear in string length.
I would have assumed the ratio to be capped by a maximum constant value?
Sorry if I misunderstood.
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.
That's a great point! I messed up here by taking an engineering approach ('the graph's fitting curve is quadratic!') rather than a first principles approach (in which both approaches are obviously linear by inspection, as you correctly point out).
My error was basically failing 'chart literacy 101'. Here's the chart that I based my conclusion on:
Looks quadratic, gets fit well by a quadratic trend line, so it must be quadratic. That's as far as my thought process went. But look at the x axis! It's not linear! If I graph the same data as a proper scatter plot (ie: on a linear x axis), we get:
which fits linear-ish (despite visually not fitting well, the R value is pretty strong).
So yeah. The improvement is actually linear as you suspect.
I'll update the docs to reflect this. Good catch!
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.
Thank you for clarifying ❤️
Based on theory and the new graph, I wonder if we won't see a glass ceiling appear if we add a couple order magnitudes on string length?
Co-authored-by: peter madsen <petermm@gmail.com>
Is this issue deadlocked? Just to be clear, from my perspective this is ready to go. Happy to do more work here if there's something missing... |
💚 💙 💜 💛 ❤️ |
Note to future other humans interested in this: http://0x80.pl/notesen/2023-03-06-swar-find-any.html |
Is adding the simdutf8 algorithm to OTP still desired? I can dedicate some time to this |
Yes! That would be extremely welcome! |
Based on the discussion on #12354, this PR adds an optional
:fast_ascii
option toString.valid?/2
(based on thebit56
algorithm discussed there). I've confirmed that this implementation yields the same benefits as observed previously:Benchmark (OTP26, ARM)