Skip to content

Conversation

sabiwara
Copy link
Contributor

@sabiwara sabiwara commented Sep 16, 2025

I tested several scenarios and noticed that the memory usage could easily go down by a 3~6x factor:
sabiwara/elixir_benches@db829b8

See individual commits.
For Enum.into(struct, map, fun) (first commit), I actually just had to remove a specialized clause that doesn't feel needed, was probably just copy-pasted from into/2.

Optimizing MapSet as a collectable feels legit, given it is a core data structure - in the same way that we have some optimizations for ranges as enumerables. Especially given the factors involved.

As a follow-up, I also plan to optimize into: MapSet.new() in comprehensions.
Given how wasteful it is right now (see bench above), it kind of defeats the purpose of trying to merge steps in one pass vs a regular pipeline, but I think we want to encourage comprehensions in such cases.

@josevalim
Copy link
Member

It would be nice if we could optimize the collectable protocol instead, so we rely less on special cases. When benchmarks ran, were protocols consolidated?

@sabiwara
Copy link
Contributor Author

It would be nice if we could optimize the collectable protocol instead, so we rely less on special cases.

That would be nice, I also don't like the idea of special cases, but there might be limits as to what we can achieve with protocols here.
Perhaps if we define some optional eager callbacks like from_list (or to_list for enums), which could be called if there is a more efficient way like with map_sets to achieve these.

When benchmarks ran, were protocols consolidated?

It should be, otherwise I think benchee would warn? I also double checked and

Collectable.__protocol__(:consolidated?) #=> true

@josevalim
Copy link
Member

I think a new callback for now, which has to be optional, would be nice. As there are cases where the conversion can be tricky. But let’s postpone it, cause the type system may impose changes to the API already. So this is good as is.

@sabiwara
Copy link
Contributor Author

I think a new callback for now, which has to be optional, would be nice. As there are cases where the conversion can be tricky. But let’s postpone it, cause the type system may impose changes to the API already. So this is good as is.

Sounds like a plan!

@sabiwara sabiwara merged commit 29e8883 into elixir-lang:main Sep 16, 2025
13 checks passed
@sabiwara sabiwara deleted the opti-into branch September 16, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants