Algoritmic improvement in Dict

[jvoigtlaender]

Replaces https://github.com/elm-lang/core/pull/255.

The essence is that calls max (RBNode_elm_builtin c k v l r) are replaced by maxWithDefault k v r, where the improvement is that the latter function is less complicated (fewer arguments, less pattern matching) and does not need an artificial (never triggered) Debug.crash case.

Two further notes:

• The function min is never called, also not exported, thus could be eliminated.
• The two functions maxWithDefault and remove_max are always called in parallel, and have the same recursion structure, so could in principle be merged into a single function.

[evancz]

Awesome, I like this one a lot more, nice refactor!

[evancz]

Awesome, I like this one a lot more, nice refactor!

[evancz]

Let's get rid of min in a PR of its own.

I don't know the code well enough to say on the second one. if you think it's better, it makes sense to me to try it.

[evancz]

Let's get rid of min in a PR of its own.

I don't know the code well enough to say on the second one. if you think it's better, it makes sense to me to try it.

[jvoigtlaender]

PR for removing min: https://github.com/elm-lang/core/pull/351

About the other potential change (merging maxWithDefault and remove_max), this would really require first having some benchmarking in place. The idea would be to use a classic tupling transformation. That is,

maxWithDefault : k -> v -> Dict k v -> (k, v)

and

remove_max : NColor -> k -> v -> Dict k v -> Dict k v -> Dict k v

would be replaced by

new_function : NColor -> k -> v -> Dict k v -> Dict k v -> ((k, v), Dict k v)

with the understanding that semantically

new_function c k v l r = (maxWithDefault k v r, remove_max c k v l r)

but actually new_function performs only one traversal instead of the two independent traversals of remove_max and maxWithDefault.

In theory, this transformation should always be an improvement, but in practice it shows (at the very least in Haskell, partly due to issues with laziness) that very often it is not. The extra tuples may lead to space overhead, extra construction and deconstruction work, etc.

[jvoigtlaender]

PR for removing min: https://github.com/elm-lang/core/pull/351

About the other potential change (merging maxWithDefault and remove_max), this would really require first having some benchmarking in place. The idea would be to use a classic tupling transformation. That is,

maxWithDefault : k -> v -> Dict k v -> (k, v)

and

remove_max : NColor -> k -> v -> Dict k v -> Dict k v -> Dict k v

would be replaced by

new_function : NColor -> k -> v -> Dict k v -> Dict k v -> ((k, v), Dict k v)

with the understanding that semantically

new_function c k v l r = (maxWithDefault k v r, remove_max c k v l r)

but actually new_function performs only one traversal instead of the two independent traversals of remove_max and maxWithDefault.

In theory, this transformation should always be an improvement, but in practice it shows (at the very least in Haskell, partly due to issues with laziness) that very often it is not. The extra tuples may lead to space overhead, extra construction and deconstruction work, etc.