Skip to content

Optimization attempt for union of maps and tuples #14205

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Optimization attempt for union of maps and tuples #14205

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 71 additions & 8 deletions lib/elixir/lib/module/types/descr.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,9 @@ defmodule Module.Types.Descr do
defp union(:bitmap, v1, v2), do: v1 ||| v2
defp union(:dynamic, v1, v2), do: dynamic_union(v1, v2)
defp union(:list, v1, v2), do: list_union(v1, v2)
defp union(:map, v1, v2), do: map_union(v1, v2)
defp union(:map, v1, v2), do: map_or_tuple_union(v1, v2)
defp union(:optional, 1, 1), do: 1
defp union(:tuple, v1, v2), do: tuple_union(v1, v2)
defp union(:tuple, v1, v2), do: map_or_tuple_union(v1, v2)

@doc """
Computes the intersection of two descrs.
Expand Down Expand Up @@ -1278,8 +1278,75 @@ defmodule Module.Types.Descr do

defp map_only?(descr), do: empty?(Map.delete(descr, :map))

# Union is list concatenation
defp map_union(dnf1, dnf2), do: dnf1 ++ (dnf2 -- dnf1)
defp map_or_tuple_union(dnf1, dnf2) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are still going to change the implementation of maps considerably, so I would suggest keeping those separate.

Something else you can do, which is what we do once we normalize, is to compute the union of two fields, if there is only field that is different. And in this case, we check for structural equality.

So this could be a more general algorithm:

  1. Do a pass on the dnf checking if they have all the same field and collecting which fields are different
  2. If any of the fields that is different is the struct field, abort
  3. If all of the fields are equal, you are done
  4. If If only one field is different, do its union
  5. If more than one field is different, check if you have all subtypes on the left or the right

This should require one pass to find fields and another to solve subtyping.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fusible_maps? function gives you a general idea of the algorithm today, although it returns a boolean, and you want something more complex. We could later on adapt the function to be shared by both union and normalize.

Copy link
Contributor Author

@sabiwara sabiwara Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are still going to change the implementation of maps considerably, so I would suggest keeping those separate.

Makes sense 👍

Something else you can do, which is what we do once we normalize, is to compute the union of two fields, if there is only field that is different. And in this case, we check for structural equality.

Interesting one, I think I see what you're saying. Only if the tags are equal though, correct?

  • %{a: integer(), b: type_of_b()} or %{a: float(), b: type_of_b()} is %{a: integer() or float(), b: type_of_b()}
  • %{..., a: integer(), b: type_of_b()} or %{..., a: float(), b: type_of_b()} is %{..., a: integer() or float(), b: type_of_b()}
  • %{..., a: integer(), b: type_of_b()} or %{a: float(), b: type_of_b()} => any other field is illegal if a is a float?

It would be nice to have a bench for this case, do you have an example in mind where we could measure its impact?

Copy link
Member

@josevalim josevalim Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, only if the tags are equal. In my mind, the most expensive parts are computing the subtyping and traversing the keys. So in my mind, the suggestions above are an improvement to this pull request because:

  1. It traverses the keys once
  2. It avoids computing the subtype (which is expensive) for the case of a single key
  3. It avoids computing the subtype for both left and right sides

In pseudo code:

case compute_keys_with_distinct_values(...) do
  :error -> # distinct keys, returns both
  [] -> # maps are the same, return left or right
  [{key, left, right}] -> # single key diverge, compute the key union and put the result back
  [{_key, left, right} | rest] = all ->
    if subtype?(left, right) do
      check_if_subtype_on_left?(rest)
    else
      check_if_subtype_on_right?(all)
    end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important point I didn't highlight in the PR description is that I'm not using subtype? (which I understand is quite expensive), but a very lightweight simple_subtype? (trivial_subtype?) which only handles trivial cases like integer() subtype_of term() or {integer(), string()} subtype_of {term(), term()} (which are the kind of cases I saw in practices in the slow examples).

I don't expect it to be costly, because it only does mostly shallow pattern/guard-based comparisons and bails as soon as something is off (different # of keys, one key missing, one key not being a trivial subtype).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I got it, but it will be slower than the equality check and, given we are traversing keys anyway, providing a more general solution than simple subtyping will yield other benefits. Basically, if we are already traversing the keys to optimize, let's optimize as much as we can?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, if we are already traversing the keys to optimize, let's optimize as much as we can?

Strongly agreed 👍
Will try to prepare some benchee bench as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in a new PR #14215

if optimized = maybe_optimize_union(dnf1, dnf2) do
optimized
else
# Union is list concatenation
# Removes duplicates in union, which should trickle to other operations.
# This is a cheap optimization that relies on structural equality.
dnf1 ++ (dnf2 -- dnf1)
end
end

defp maybe_optimize_union([{tag1, pos1, []}] = dnf1, [{tag2, pos2, []}] = dnf2) do
# Avoid the explosion for unions of very similar maps or tuples,
# by returning only one if it contains the other
# e.g. %{a: integer()} or %{..., a: term()} -> %{..., a: term()}
cond do
map_or_tuple_simple_subtype?(tag1, pos1, tag2, pos2) -> dnf2
map_or_tuple_simple_subtype?(tag2, pos2, tag1, pos1) -> dnf1
true -> nil
end
end

defp maybe_optimize_union(_, _), do: nil

defp map_or_tuple_simple_subtype?(_, _, :open, pos2) when pos2 == %{}, do: true

defp map_or_tuple_simple_subtype?(:closed, pos1, :closed, pos2)
when map_size(pos1) == map_size(pos2) do
all_key_simple_subtypes?(pos1, pos2)
end

defp map_or_tuple_simple_subtype?(_, pos1, :open, pos2) when map_size(pos1) >= map_size(pos2) do
all_key_simple_subtypes?(pos1, pos2)
end

defp map_or_tuple_simple_subtype?(_, _, _, _), do: false

defp all_key_simple_subtypes?(pos1, pos2) do
Enum.all?(pos1, fn {key, type1} ->
case pos2 do
%{^key => type2} -> simple_subtype?(type1, type2)
_ -> false
end
end)
end

defp simple_subtype?(_, :term), do: true
defp simple_subtype?(same, same), do: true

defp simple_subtype?(type1, type2) when map_size(type1) == 1 and map_size(type2) == 1 do
case {type1, type2} do
{%{atom: _}, %{atom: {:negation, neg}}} when neg == %{} ->
true

{%{bitmap: bitmap1}, %{bitmap: bitmap2}} ->
(bitmap1 &&& bitmap2) === bitmap2

{%{map: [{tag1, pos1, []}]}, %{map: [{tag2, pos2, []}]}} ->
map_or_tuple_simple_subtype?(tag1, pos1, tag2, pos2)

{%{tuple: [{tag1, pos1, []}]}, %{tuple: [{tag2, pos2, []}]}} ->
map_or_tuple_simple_subtype?(tag1, pos1, tag2, pos2)

_ ->
false
end
end

defp simple_subtype?(_, _), do: false

# Given two unions of maps, intersects each pair of maps.
defp map_intersection(dnf1, dnf2) do
Expand Down Expand Up @@ -2049,10 +2116,6 @@ defmodule Module.Types.Descr do
end
end

# Removes duplicates in union, which should trickle to other operations.
# This is a cheap optimization that relies on structural equality.
defp tuple_union(left, right), do: left ++ (right -- left)

defp tuple_to_quoted(dnf, opts) do
dnf
|> tuple_simplify()
Expand Down
Loading