Optimize String.replace_* functions

[michalmuskala]

• Skip unnecessary matches on the "rest" size
• Don't prepend/append empty strings

The second one is especially important - doing "" <> str will force a copy of
the string. Checking, if the string is empty first, allows us to skip this copy.
Using replace functions with empty replacements is rather common - one example
is the String.trim* family of functions, but even in user code it's quite
frequent.
Removing excessive copy means the speedup can be made arbitrarily large
depending on input. The slowdown for cases with non-empty strings couldn't be
reliably measured.

[josevalim]

Have you verified that an empty string actually copies the left/right side? What about the ++ operator? It feels those improvements should be rather done at the VM level? -- *José Valim* www.plataformatec.com.br Skype: jv.ptec Founder and Director of R&D

[michalmuskala]

Yes, it does copy.

Given the following module:

defmodule Test do
  def test do
    str = :binary.copy("a", 10000)
    a = binary_part(str, 10, 1000)
    IO.inspect :binary.referenced_byte_size(a)
    IO.inspect :binary.referenced_byte_size(a <> "")
  end
end

Running Test.test() produces the following output:

10000
2000

This means a new binary is created and the data copied.

Optimizing this at VM level might actually be quite hard given how binaries work. I can try reporting it to the OTP team.

[michalmuskala]

Also - the Travis failure seems unrelated.

[michalmuskala]

What about the ++ operator?

I missed this question earlier. In general, it will always copy the thing on the left and never the one on the right, so [] ++ list is fine, I have no idea about list ++ [] - I don't even have an idea how to check this.

[josevalim]

defmodule Foo do
  def bar do
    list = List.duplicate(0, 10000000)
    :timer.tc(fn -> list ++ [] end) |> elem(0) |> IO.inspect
    :timer.tc(fn -> [] ++ list end) |> elem(0) |> IO.inspect
    :timer.tc(fn -> list ++ [] end) |> elem(0) |> IO.inspect
    :timer.tc(fn -> [] ++ list end) |> elem(0) |> IO.inspect
  end
end

When the empty list is on the right side, we can say that it at least traverses the whole left side. It is unclear if it copies it though. I would say that traversing is unecessary since the operation should fail if it is an improper list.

[whatyouhide]

I think the names should be prepend_unless_empty and append_unless_empty if we go with this. Any particular reason why not going with a concat function that checks for both sides then?

[josevalim]

@whatyouhide I like the specific functions, so I would go with prepend_unless_emptyand append_unless_empty.

[michalmuskala]

Updated

[josevalim]

❤️ 💚 💙 💛 💜