Stripe.URI Efficiency

[DavidAntaramian]

The Stripe.URI module code should be reviewed. It's a small issue, but right now it uses ++ with the left operand as the accumulator and the right operand as the added value. ++ is implemented such that the left operand is always copied, so our accumulator is always copied (which is what we don't want). See The Eight Myths of Erlang Performance

[tlvenn]

Yes could leverage iodata and to some extend maybe a bit of https://github.com/benoitc/hackney/blob/master/doc/hackney_url.md

[DavidAntaramian]

Yeah. I have an implementation using iodata in another system. I may just copy it over.

[fastjames]

If your goal is to remove the ++ operation (which I understand for performance reasons) wouldn't the simple fix be something like this?

diff --git a/lib/stripe/uri.ex b/lib/stripe/uri.ex
index 7aa1bd9..5cfc55a 100644
--- a/lib/stripe/uri.ex
+++ b/lib/stripe/uri.ex
@@ -48,9 +48,9 @@ defmodule Stripe.URI do
     Enum.map_join values, "&", fn {key, value} ->
       cond do
         Enumerable.impl_for(value) ->
-          pair(root, parents ++ [key], value)
+          pair(root, [key | parents], value)
         true ->
-          build_key(root, parents ++ [key]) <> URI.encode_www_form(to_string(value))
+          build_key(root, Enum.reverse([key | parents])) <> URI.encode_www_form(to_string(value))
       end
     end
   end

or do you want to avoid Enum.reverse as well? I haven't read the source on that one but I assumed one round of it was OK.

[DavidAntaramian]

The Enum.reverse is fine. The problem with ++ is the way the memory allocation and copy-on-write mechanism works with the VM.

The solution that you have given looks like an improvement, but I think the overall logic (such as use of Enumerable.impl_for/1) can be improved.

[joshsmith]

@fastjames given that I think if you want to open a PR to spike this out and see if @DavidAntaramian or someone else can provide feedback on that, I'd be really thankful!

[fastjames]

Pull request submitted: #184