Add nonidempotent_union to maps#26
Conversation
Currently, the `union` operation with a non-idempotent reconciliation function is `slow_merge`. This adds `nonidempotent_union` which has the following advantages over `slow_merge`: - The interface is easier to use and look more similar to the union operation that we can see in other datastructures. - The implementation is faster as it doesn't traverse uncommon subtrees. Also, the reconciliation function is called once for each common binding compared to once per bindings in both maps. The implementation is basically the same as idempotent_union with physical equality checks removed.
dlesbre
left a comment
dlesbre
left a comment
There was a problem hiding this comment.
Thanks for this PR! It looks good overall, I just have a suggestion of a case where we can maintain node sharing during nonidempotent_union.
src/functors.ml
Outdated
| let newv = f.f ~key ~old ~value in | ||
| leaf key newv |
There was a problem hiding this comment.
I think we should still have a check for physical equality here. The original function had two checks, one before the call to f and one after. The first one obviously needs to go but the second can still be used here to maintain sharing when f returns an unmodified value (which I suspect can be quite common in practice).
There was a problem hiding this comment.
Done! I was having second thought about the duplicated code so I implemented the nonidempotent insert_for_union with a boolean flag. What do you think ?
There was a problem hiding this comment.
Thanks for the changes, its nice that you managed to merge both functions
|
Thanks for the PR! I remember that the performance of PatriciaTree was not satisfactory on union, and if this is what needs to be done to be on par ethe others (and if not, what can we do to improve performances)? |
This adds one of the optimisation from `idempotent_union` to `nonidempotent_union`. The duplicated code for the `insert_for_union` branches is factorized and a boolean argument is used to distinguish when to skip calling `f.f`. This reduces the amount of code and highlights better the difference in implementation between idempotent and nonidempotent union, at the cost of a boolean argument.
3 participants
Currently, the
unionoperation with a non-idempotent reconciliation function isslow_merge.This adds
nonidempotent_unionwhich has the following advantages overslow_merge:The interface is easier to use and look more similar to the union operation that we can see in other datastructures.
The implementation is faster as it doesn't traverse uncommon subtrees. Also, the reconciliation function is called once for each common binding compared to once per bindings in both maps.
The implementation is basically the same as
idempotent_unionwith physical equality checks removed.