MapSet Optimizations #4415

benwilson512 · 2016-03-20T19:01:59Z

Overview

Hey folks!

Based upon discoveries made during my little foray into map performance characteristics a few weeks ago I thought I'd give MapSet a work over. I was also very curious about the circumstances under which MapSet out performs :lists.subtract

Warning: Be careful when benchmarking -- or :lists.subtract. The compiler (I think it's the compiler) does some crazy optimizations when operating on list literals. Always create a module / function like MyList.subtract/2 and call that instead, otherwise the result will be pre-computed at compile time.

General Changes

Many of the current functions use :maps.fold. Under the covers :maps.fold calls :maps.to_list first, and then :lists.foldl. In many cases :maps.fold uses Map.put to accumulate desired results in a new map.

This gives us three general opportunities for improvement.

By far the largest improvement comes from first accumulating {key, value} pairs into a list, and then using :maps.from_list instead of consecutive calls to Map.put/2. This is not only faster, but (from what I've been told) produces less garbage for the GC. If anyone knows of a good way to quantify the amount of garbage produced I'd love to hear it.
:maps.to_list can be improved upon by simply using Map.keys. Not only does Map.keys happen to be faster, it also improves code clarity because we only care about the keys anyway.
:lists.fold can be improved upon by manually recursing through lists produced by Map.keys and calling other functions as needed. While this does make the code more verbose, it tends to get us about a 20% improvement.

Functions

Benchmarking notes: Y axis is in micro seconds.

`MapSet.difference/2`

Benchmark: https://github.com/benwilson512/elixir-profiling/blob/master/bench/mapset_difference_bench.exs

MapSet.difference(set1, set2) is the only function that had any algorithmic changes. The current approach iterates through set2 and deletes each item found from set1.

I realized that when set2 is larger than set1 this no longer makes any sense. Instead it is faster to iterate through each item in set1 and only accumulate those items which do not also exist in set2.

After testing, I determined that it becomes faster to iterate through set1 when set2 is at least half the size of set1.

`MapSet.intersection/2`

Benchmark: https://github.com/benwilson512/elixir-profiling/blob/master/bench/mapset_intersection_bench.exs

Accumulate in a list, manual recursion, Map.keys. These small changes produce a surprisngly substantial improvement.

`MapSet.disjoint/2`

Benchmark: https://github.com/benwilson512/elixir-profiling/blob/master/bench/mapset_disjoint_bench.exs

The previous version iterated through the smallest set and would throw in the event that a given key was also found in the other set.

This version uses Map.keys and manual recursion to achieve functional flow control and superior performance.

`MapSet.new`

Benchmark: https://github.com/benwilson512/elixir-profiling/blob/master/bench/mapset_new_bench.exs

Manual recursion, list accumulation.

While I was at it, I took a similar approach with Map.new/1,2.

`MapSet.disjoint?`

Comparison to `--`

I'm still working on this, I figured I should get some feedback on my changes here before going further on this because any major changes we make have the potential to change my analysis here.

benwilson512 · 2016-03-20T19:03:09Z

lib/elixir/lib/map.ex

-    Enum.reduce(enumerable, %{}, fn {k, v}, acc -> put(acc, k, v) end)
+    enumerable
+    |> Enum.reduce([], &[&1 | &2])
+    |> :lists.reverse


This is necessary because even though Maps are unordered, if we do Map.new([a: 1, a: 2]) the current behaviour is that we get %{a: 2}. Without reversing we would get %{a: 1}

Why not just Enum.to_list?

Ah whoops I just edited this from the MapSet version, that would indeed make sense.

michalmuskala · 2016-03-20T19:34:32Z

lib/elixir/lib/map.ex

+    enumerable
+    |> Enum.reduce([], &[transform.(&1) | &2])
+    |> :lists.reverse
+    |> :maps.from_list


We could compare this with Enum.into(enumerable, %{}, transform).

If one is faster than the other, both should be probably changed to be the same implementation.

Are you suggesting a fast track for Enum.into/2 and maps? The regular collectable implementation for maps can't benefit from this, because it only has one item at a time. :maps.from_list only works when you know all the items you want to put into the map ahead of time.

Collectable for maps could accumulate a list and call :maps.from_list/1 at the end instead of accumulating a map if that's faster.

It was my understanding that each iteration in collectable had to return a map, but if not then yes we should do that to improve Enum.into. However, I don't think we should use Enum.into here. The protocol overhead would be definitely noticeable.

Actually, Enum.into/2 optimizes a bunch of cases. So we should very likely call Enum.into/2 instead and make sure we optimize those cases there. :)

And we should optimize the collectable to. To be clear and avoid confusion:

Enum.into/2 inlines some code when it notices the second argument is a map (and not a struct). We can rely on it.

Regardless of Enum.into/2, we can also optimize collectable.

In particular, Enum.into/2 should optimize into a single :maps.from_list call if the first argument is a list and the second is a map (not a struct). I believe we already do it today but, if we don't, we certainly should.

From what I've seen we don't do any optimizations for Enum.into an empty (or very small) map. I do believe that the best place for optimization is the Enum.into module itself, rather than the collectable protocol implementation for maps or mapsets.

So, I think we have another reason to avoid Enum.into in map: It's nicer to optimize Enum.into in terms of Map.new, rather than the other way around. For example. here's my proposed change to Enum.into for empty maps:

def into(enumerable, %{} = map) when map_size(map) == 0 do Map.new(enumerable) end

Alternatively, we could have Enum.into do what my Map.new does now, and have Map.new use Enum.into. Thoughts?

Those are good points. It is OK to Map.new and Enum.into apart for now then.

josevalim · 2016-03-20T19:54:10Z

Please go ahead with those changes. Using Enum.to_list would clean things up a bit but this is definitely the way to go. ❤️ 💚 💙 💛 💜

benwilson512 · 2016-03-20T20:03:08Z

General note: I realize that some of these changes are a bit more verbose, and I promise that I'm not a "faster at all costs" kinda guy. However when it comes to core language data structures I really do think that every bit counts, which is why you see me using a lot of manual recursion over Enum. I've tested the Enum versions locally and in most cases we gain at least 20% by recursing manually.

Part of this is the manual recursion, but I'm convinced a bigger part is that we get to avoid anonymous functions in the hot path. If it was only 2% I wouldn't care, but at 20% it really matters.

josevalim · 2016-03-20T20:08:40Z

Yup, it is ok for us to have "verbose" code if it means everyone else does not have to.

benwilson512 · 2016-03-20T20:36:40Z

Preliminary Map module improvements:
Done by making a map of 10,000 items, and then dropping / taking / splitting 5,000 items

                          Before   After
Map.drop           1000   1948.48  1833.82
Map.take           1000   2181.71  1108.00
Map.split           500   5095.14  1122.09

benwilson512 · 2016-03-20T20:37:14Z

I'm gonna keep making regular commits and will squash when we're done.

nussjustin · 2016-03-20T21:04:44Z

lib/elixir/lib/map_set.ex

+    %MapSet{map: Map.drop(map1, Map.keys(map2))}
+  end
+
+  def filter_not_in(keys, map2) do


I think this should be private.

Indeed, also it'd better to define it as bodiless clause def filter_not_in(keys, map2, acc \\ []).

benwilson512 · 2016-03-20T22:16:12Z

Anyone know why the commit that added Enum.into specialization broke compilation? I didn't notice locally at first because the regular make test worked, it's only when I ran make clean test that it blew up. It seems like there's some kind of compile time dependency on Map.new in the enum module?

lexmag · 2016-03-20T22:27:44Z

Anyone know why the commit that added Enum.into specialization broke compilation?

Compilation fails because of %MapSet{} usage, it expects MapSet module to be available during compilation but we haven't compiled it yet.
To fix that we should use %{__struct__: MapSet}.

benwilson512 · 2016-03-20T22:32:57Z

Aha, thanks!

benwilson512 · 2016-03-21T02:55:39Z

Commits squashed. I'm pretty happy with where this is at. I think there are some things to think about as far as Enum.into and the collectable protocol ar concerned but those are definitely orthogonal, and since they may have implications beyond MapSet, I think deserve their own PR / discussion.

Any outstanding comments / critiques?

josevalim · 2016-03-21T09:24:21Z

lib/elixir/lib/map.ex

@@ -77,7 +77,9 @@ defmodule Map do
  """
  @spec new(Enum.t) :: map
  def new(enumerable) do


What if a map is given as argument?

ah yeah we should just return the same map in that case.

Just be careful because the map must not be a struct.

Good point.

josevalim · 2016-03-21T09:27:54Z

I have dropped some final comments and we are ready to go!

lexmag · 2016-03-21T09:30:36Z

lib/elixir/lib/map_set.ex

@@ -52,7 +57,30 @@ defmodule MapSet do
  """
  @spec new(Enum.t, (term -> term)) :: t
  def new(enumerable, transform) do
-    Enum.reduce(enumerable, %MapSet{}, &put(&2, transform.(&1)))
+    map =
+    enumerable


Misindentation here, it needs to be:

map = enumerable |> Enum.to_list

lexmag · 2016-03-21T09:42:04Z

lib/elixir/lib/map_set.ex

+
+  defp filter_not_in(keys, map2, acc \\ [])
+  defp filter_not_in([], _map2, acc), do: :maps.from_list(acc)
+  defp filter_not_in([k | rest], map2, acc) do


I'd use key here instead of k to match existing style, also feels more readable.

lexmag · 2016-03-21T23:46:16Z

lib/elixir/lib/map.ex

@@ -425,7 +441,12 @@ defmodule Map do
  """
  @spec drop(map, [key]) :: map
  def drop(map, keys) do
-    Enum.reduce(keys, map, &delete(&2, &1))
+    do_drop(keys, map)


I assume the @spec for this function is wrong as well, we do need to handle any enumerable, right?
Same with split/2.

benwilson512 · 2016-03-22T00:12:05Z

One thing that is conspicuously absent here are tests. split, take and drop only have doc tests, and those cases don't cover enumerable input. Should I fill out the map_test.exs with relevant cases or is relying on doc tests and fleshing out the examples the way we should go?

josevalim · 2016-03-22T08:27:31Z

Both approaches are fine. Feel free to add more tests or add doctests or
both. :)

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

On Tue, Mar 22, 2016 at 1:12 AM, Ben Wilson notifications@github.com
wrote:

One thing that is conspicuously absent here are tests. split, take and
drop only have doc tests, and those cases don't cover enumerable input.
Should I fill out the map_test.exs with relevant cases or is relying on
doc tests and fleshing out the examples the way we should go?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#4415 (comment)

benwilson512 · 2016-03-22T19:33:02Z

Tests are in.

josevalim · 2016-03-23T10:52:06Z

❤️ 💚 💙 💛 💜

benwilson512 reviewed Mar 20, 2016
View reviewed changes

benwilson512 force-pushed the mapset-optimizations branch from b36a5a3 to e7b3731 Compare March 20, 2016 19:06

michalmuskala reviewed Mar 20, 2016
View reviewed changes

benwilson512 force-pushed the mapset-optimizations branch from e7b3731 to 5e0ffa0 Compare March 20, 2016 20:47

nussjustin reviewed Mar 20, 2016
View reviewed changes

benwilson512 force-pushed the mapset-optimizations branch from af33920 to 3545576 Compare March 21, 2016 02:52

benwilson512 force-pushed the mapset-optimizations branch from 3545576 to 1a3ef8f Compare March 21, 2016 03:53

josevalim reviewed Mar 21, 2016
View reviewed changes

lexmag reviewed Mar 21, 2016
View reviewed changes

benwilson512 force-pushed the mapset-optimizations branch from 1a3ef8f to 63dcba9 Compare March 21, 2016 14:10

lexmag reviewed Mar 21, 2016
View reviewed changes

benwilson512 force-pushed the mapset-optimizations branch from 63dcba9 to f343409 Compare March 22, 2016 00:07

MapSet Optimizations

11f046a

benwilson512 force-pushed the mapset-optimizations branch from f343409 to 11f046a Compare March 22, 2016 16:57

josevalim merged commit f4f2688 into elixir-lang:master Mar 23, 2016

MapSet Optimizations #4415

MapSet Optimizations #4415

Conversation

benwilson512 commented Mar 20, 2016

Overview

General Changes

Functions

MapSet.difference/2

MapSet.intersection/2

MapSet.disjoint/2

MapSet.new

MapSet.disjoint?

Comparison to --

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim Mar 20, 2016 via email

Choose a reason for hiding this comment

josevalim commented Mar 20, 2016

benwilson512 commented Mar 20, 2016

josevalim commented Mar 20, 2016

benwilson512 commented Mar 20, 2016

benwilson512 commented Mar 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benwilson512 commented Mar 20, 2016

lexmag commented Mar 20, 2016

benwilson512 commented Mar 20, 2016

benwilson512 commented Mar 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Mar 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benwilson512 commented Mar 22, 2016

josevalim commented Mar 22, 2016

benwilson512 commented Mar 22, 2016

josevalim commented Mar 23, 2016

`MapSet.difference/2`

`MapSet.intersection/2`

`MapSet.disjoint/2`

`MapSet.new`

`MapSet.disjoint?`

Comparison to `--`