New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix performance issue with List.sortBy #631

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@jvoigtlaender
Contributor

jvoigtlaender commented May 29, 2016

The issue fixed here is as follows, testable at http://elm-lang.org/try.

The following program:

import Html exposing (button, text)
import Html.App exposing (program)
import Html.Events exposing (onClick)
import Task exposing (succeed, andThen, perform)
import Time exposing (Time, now)

f n =
    if n < 2 then
        1
    else
        f (n - 1) + f (n - 2)

list =
    [30..35]

sortBy f =
    List.sortBy f

main =
    program { init = ( Nothing, Cmd.none ), view = view, update = update, subscriptions = \_ -> Sub.none }

view model =
    button [ onClick Click ] [ text (toString model) ]

type Msg a
    = Click
    | Finished Time a

update msg model =
    case msg of
        Click ->
            ( model
            , perform identity identity
                <| now
                `andThen` \t ->
                            succeed (sortBy f (List.reverse list))
                                `andThen` \x ->
                                            now
                                                `andThen` \t' -> succeed <| Finished (t' - t) x
            )

        Finished dt x ->
            ( Just ( dt, x ), Cmd.none )

needs, in the browser on my machine, almost 4 seconds after each button click to show the new result. (The time taken, in milliseconds, will be printed as the first component of the pair in the button text.)

The problem is that f is called far too often on individual elements of list.

By just swapping in the following definition instead of the current List.sortBy:

sortBy f =
    List.map snd << List.sortBy fst << List.map (\a -> ( f a, a ))

the 4 seconds from above are reduced to well under 1 second.

This is not just about a constant factor time difference. By changing f and/or list, the "old" List.sortBy can be arbitrarily (even asymptotically) more expensive than the "new" version. The converse is not true. The new implementation is guaranteed to never be asymptotically worse than the current List.sortBy.

The new version is also how the equivalent functionality is implemented in the Haskell standard library.

This pull request makes it so that Elm's List.sortBy uses the more efficient implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment