elixir icon indicating copy to clipboard operation
elixir copied to clipboard

Reimplement and optimize intersection of map types

Open sabiwara opened this issue 9 months ago • 3 comments

Not 100% confident with my reverse-engineering, but at least all tests are passing.

I haven't benchmarked it properly, but I'm quite confident it should be faster since we are doing one loop for each approach. This simplified example benchmark shows a single-pass on an iterator like :maps.merge_with is much faster than the for ++ |> Map.new.

If my understanding is correct:

  • open - open: this is a simple merge_with, types that are just in one map can be kept as is
  • closed - closed: this is a simple intersect_with, but we should throw if some keys weren't in both
  • open -closed: we can just loop on open keys, check they are all present in the closed one, and update closed as an acc

I completely removed the default stuff though, so if I broke something that was not tested, happy to add the tests and rethink my approach accordingly.

sabiwara avatar May 01 '24 20:05 sabiwara

This looks great! In the future we want to allow the fallback to be any type but, as an optimization, this works great. :)

josevalim avatar May 01 '24 21:05 josevalim

This looks great! In the future we want to allow the fallback to be any type but, as an optimization, this works great. :)

OK, so the current approach might fall apart, because we might need to loop on both maps to achieve that as the current version is doing? Maybe we should hold off until we have implemented this part?

sabiwara avatar May 02 '24 16:05 sabiwara

@sabiwara there will only be two fallbacks: from literal atoms to atom() and from everything to term() (:open is equivalent to all terms falling back to term()). So I am pretty sure we can still optimize it. As long as atom() falls back to term and term() fallsback to term, the optimized paths for open-open are the same. Similar for closed and closed (as long as both atom() and term() fallback to none).

josevalim avatar May 02 '24 17:05 josevalim