Optimize URI form and RFC 3986 query encoding with SWAR - #15733
Conversation
Use a guarded 56-bit fast path with compact scalar tail handling and an isolated fallback scanner. Fast-path empty inputs before allocating the binary builder, and cover SWAR lane and length boundaries with focused differential tests. Assisted-by: Codex:GPT-5
|
Thank you but this implementation is much more complex than the other SWAR ones, for a code that is not that sensitive, so I am not sure if it is worth it. |
NelsonVides
left a comment
There was a problem hiding this comment.
Couple of comments but also, @josevalim mentioned SWAR being too complicated here, also considering normal input for this function is really just a few tens of bytes, unlike JSON which can see hundreds of MBs of input.
I've tried to offer a couple of points where we could simplify the logic, and I could probably think of more, but if it is still too much, perhaps we can still go for one optimisation: the for ... into: loop. As I mentioned in the forum thread, that compiles to a comprehension with a function call, a temporary binary allocation, and a case-do comparison, while we could instead rewrite the loop into a tail-recursive function as in something like
defguardp unreserved_char?(character)
when character in ?0..?9 or character in ?a..?z or character in ?A..?Z or
character in ~c"~_-."
# char_unreserved?/1 becomes: unreserved_char?(character)
# encode_kv_pair/:rfc3986 becomes: encode_unreserved(Kernel.to_string(key), :percent) <> ...
def encode_www_form(string) when is_binary(string) do
encode_unreserved(string, "", :www_form)
end
defp encode_unreserved(string, mode), do: encode_unreserved(string, "", mode)
defp encode_unreserved(<<?\s, rest::binary>>, acc, :www_form) do
encode_unreserved(rest, <<acc::binary, ?+>>, :www_form)
end
defp encode_unreserved(<<byte, rest::binary>>, acc, mode) when unreserved_char?(byte) do
encode_unreserved(rest, <<acc::binary, byte>>, mode)
end
defp encode_unreserved(<<byte, rest::binary>>, acc, mode) do
encode_unreserved(rest, <<acc::binary, ?%, hex(bsr(byte, 4)), hex(band(byte, 15))>>, mode)
end
defp encode_unreserved(<<>>, acc, _mode), do: accWhich in my local benchmarks (macbook M4) gives me 90% of the performance improvements this branch gives (the other 10% being SWAR specifically). Perhaps we can split this PR in two and first of all propose the loop rewrite, which should be uncontroversial, and if that works then we can discuss the SWAR and think how else we could simplify until it's worth it?
| # Consume through the first disallowed byte instead of checking overlapping words. | ||
| defp encode_unreserved_fallback(<<byte1, byte2, rest::binary>>, acc, mode) | ||
| when byte_size(rest) >= 5 and not unreserved_char?(byte2) do | ||
| encoded = encode_unreserved_byte(byte2, mode) | ||
| encode_unreserved(rest, <<acc::binary, byte1, encoded::binary>>, mode) | ||
| end | ||
|
|
||
| defp encode_unreserved_fallback(<<byte1, byte2, byte3, rest::binary>>, acc, mode) | ||
| when byte_size(rest) >= 4 and not unreserved_char?(byte3) do | ||
| encode_unreserved( | ||
| rest, | ||
| <<acc::binary, byte1, byte2, encode_unreserved_byte(byte3, mode)::binary>>, | ||
| mode | ||
| ) | ||
| end | ||
|
|
||
| defp encode_unreserved_fallback(<<byte1, byte2, byte3, byte4, rest::binary>>, acc, mode) | ||
| when byte_size(rest) >= 3 and not unreserved_char?(byte4) do | ||
| encode_unreserved( | ||
| rest, | ||
| <<acc::binary, byte1, byte2, byte3, encode_unreserved_byte(byte4, mode)::binary>>, | ||
| mode | ||
| ) | ||
| end | ||
|
|
||
| defp encode_unreserved_fallback(<<byte1, byte2, byte3, byte4, byte5, rest::binary>>, acc, mode) | ||
| when byte_size(rest) >= 2 and not unreserved_char?(byte5) do | ||
| encode_unreserved( | ||
| rest, | ||
| <<acc::binary, byte1, byte2, byte3, byte4, encode_unreserved_byte(byte5, mode)::binary>>, | ||
| mode | ||
| ) | ||
| end | ||
|
|
||
| defp encode_unreserved_fallback( | ||
| <<byte1, byte2, byte3, byte4, byte5, byte6, rest::binary>>, | ||
| acc, | ||
| mode | ||
| ) | ||
| when byte_size(rest) >= 1 and not unreserved_char?(byte6) do | ||
| encode_unreserved( | ||
| rest, | ||
| <<acc::binary, byte1, byte2, byte3, byte4, byte5, | ||
| encode_unreserved_byte(byte6, mode)::binary>>, | ||
| mode | ||
| ) | ||
| end | ||
|
|
||
| defp encode_unreserved_fallback( | ||
| <<byte1, byte2, byte3, byte4, byte5, byte6, byte7, rest::binary>>, | ||
| acc, | ||
| mode | ||
| ) do | ||
| encode_unreserved( | ||
| rest, | ||
| <<acc::binary, byte1, byte2, byte3, byte4, byte5, byte6, | ||
| encode_unreserved_byte(byte7, mode)::binary>>, | ||
| mode | ||
| ) | ||
| end |
There was a problem hiding this comment.
Maybe generate the repetitive clauses.
| # Consume through the first disallowed byte instead of checking overlapping words. | |
| defp encode_unreserved_fallback(<<byte1, byte2, rest::binary>>, acc, mode) | |
| when byte_size(rest) >= 5 and not unreserved_char?(byte2) do | |
| encoded = encode_unreserved_byte(byte2, mode) | |
| encode_unreserved(rest, <<acc::binary, byte1, encoded::binary>>, mode) | |
| end | |
| defp encode_unreserved_fallback(<<byte1, byte2, byte3, rest::binary>>, acc, mode) | |
| when byte_size(rest) >= 4 and not unreserved_char?(byte3) do | |
| encode_unreserved( | |
| rest, | |
| <<acc::binary, byte1, byte2, encode_unreserved_byte(byte3, mode)::binary>>, | |
| mode | |
| ) | |
| end | |
| defp encode_unreserved_fallback(<<byte1, byte2, byte3, byte4, rest::binary>>, acc, mode) | |
| when byte_size(rest) >= 3 and not unreserved_char?(byte4) do | |
| encode_unreserved( | |
| rest, | |
| <<acc::binary, byte1, byte2, byte3, encode_unreserved_byte(byte4, mode)::binary>>, | |
| mode | |
| ) | |
| end | |
| defp encode_unreserved_fallback(<<byte1, byte2, byte3, byte4, byte5, rest::binary>>, acc, mode) | |
| when byte_size(rest) >= 2 and not unreserved_char?(byte5) do | |
| encode_unreserved( | |
| rest, | |
| <<acc::binary, byte1, byte2, byte3, byte4, encode_unreserved_byte(byte5, mode)::binary>>, | |
| mode | |
| ) | |
| end | |
| defp encode_unreserved_fallback( | |
| <<byte1, byte2, byte3, byte4, byte5, byte6, rest::binary>>, | |
| acc, | |
| mode | |
| ) | |
| when byte_size(rest) >= 1 and not unreserved_char?(byte6) do | |
| encode_unreserved( | |
| rest, | |
| <<acc::binary, byte1, byte2, byte3, byte4, byte5, | |
| encode_unreserved_byte(byte6, mode)::binary>>, | |
| mode | |
| ) | |
| end | |
| defp encode_unreserved_fallback( | |
| <<byte1, byte2, byte3, byte4, byte5, byte6, byte7, rest::binary>>, | |
| acc, | |
| mode | |
| ) do | |
| encode_unreserved( | |
| rest, | |
| <<acc::binary, byte1, byte2, byte3, byte4, byte5, byte6, | |
| encode_unreserved_byte(byte7, mode)::binary>>, | |
| mode | |
| ) | |
| end | |
| # Consume through the first disallowed byte instead of checking overlapping words. | |
| for n <- 2..6 do | |
| leading = for i <- 1..(n - 1), do: Macro.var(:"byte#{i}", __MODULE__) | |
| defp encode_unreserved_fallback(<<unquote_splicing(leading), byte, rest::binary>>, acc, mode) | |
| when not unreserved_char?(byte) do | |
| encode_unreserved( | |
| rest, | |
| <<acc::binary, unquote_splicing(leading), encode_unreserved_byte(byte, mode)::binary>>, | |
| mode | |
| ) | |
| end | |
| end | |
| # Byte 7 is the offender by elimination, so this clause needs no guard. | |
| defp encode_unreserved_fallback( | |
| <<byte1, byte2, byte3, byte4, byte5, byte6, byte7, rest::binary>>, | |
| acc, | |
| mode | |
| ) do | |
| encode_unreserved( | |
| rest, | |
| <<acc::binary, byte1, byte2, byte3, byte4, byte5, byte6, | |
| encode_unreserved_byte(byte7, mode)::binary>>, | |
| mode | |
| ) | |
| end |
| defp encode_unreserved(string, mode) when byte_size(string) < @swar_threshold do | ||
| case string do | ||
| <<>> -> string | ||
| _ -> encode_unreserved_small(string, "", mode) | ||
| end | ||
| end | ||
|
|
||
| defp encode_unreserved(string, mode), do: encode_unreserved(string, "", mode) |
There was a problem hiding this comment.
| defp encode_unreserved(string, mode) when byte_size(string) < @swar_threshold do | |
| case string do | |
| <<>> -> string | |
| _ -> encode_unreserved_small(string, "", mode) | |
| end | |
| end | |
| defp encode_unreserved(string, mode), do: encode_unreserved(string, "", mode) | |
| defp encode_unreserved(<<>>, _mode), | |
| do: "" | |
| defp encode_unreserved(string, mode) when byte_size(string) < @swar_threshold, | |
| do: encode_unreserved_small(string, "", mode) | |
| defp encode_unreserved(string, mode), | |
| do: encode_unreserved(string, "", mode) |
plus formatting
|
Thank you @josevalim and @NelsonVides . @NelsonVides based on your feedback my local agent implemented a version that is indeed much simpler and has most of the performance benefits so I think it's best if I close this version. @NelsonVides do you plan to open a PR with that? Would be great I think. |
Assisted-by: Codex CLI:GPT-5.6 Sol
Use a guarded 56-bit fast path with compact scalar tail handling and an isolated fallback scanner. Fast-path empty inputs before allocating the binary builder, and cover SWAR lane and length boundaries with focused differential tests.
@NelsonVides it would be great if you would review it.
Elixir Forum discussion: https://elixirforum.com/t/help-swar-optimize-uri-encode-www-form-1/76227
Benchmark:
Average of medians from two alternating runs: