Optimize for comprehensions into: MapSet #14796
Conversation
| build_into(Ann, Clauses, Expr, ?empty_map_set_pattern = _Into, Uniq, S) -> | ||
| InnerFun = fun(InnerExpr, InnerAcc) -> {cons, Ann, InnerExpr, InnerAcc} end, | ||
| {ReduceExpr, SR} = build_reduce(Ann, Clauses, InnerFun, Expr, {nil, Ann}, Uniq, S), | ||
| {?remote(Ann, 'Elixir.MapSet', new, [ReduceExpr]), SR}; |
There was a problem hiding this comment.
I think we could even optimize this further by changing the comprehension to return {Item, true} and then building the structure directly.
There was a problem hiding this comment.
Oh, do you mean this:
%MapSet{map: :maps.from_list(Enum.reduce([1, 2, 3], [], fn x, var3 -> [{x, []} | var3] end))}Makes sense!
({Item, []}, not {Item, true}, right?)
There was a problem hiding this comment.
OK, interesting.
Done in c713cde, but while it speeds things up, it also seems to increase memory significantly. Maybe not worth it? sabiwara/elixir_benches@14c0876
(maybe because you implemented :sets.from_list(version: 2) efficiently as a BIF, and it doesn't need all these tuples?)
There was a problem hiding this comment.
Will revert to the previous version and merge, WDYT?
There was a problem hiding this comment.
Keep the new version but use from_keys without the tuple. Although I guess that’s the same as MapSet.new anyway? So yeah, please revert!
This reverts commit c713cde.
Follow-up for #14774.
Fixes the discrepancy found in the previous benchmark: sabiwara/elixir_benches@cd1c9a3.
Before
After
Note: this is basically the same as building a list, but we don't need the
lists:reversestep and replace it withMapSet.new/1.