Skip to content

Simplify identity proposal #36

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

sebsheep
Copy link
Contributor

@sebsheep sebsheep commented Mar 6, 2021

No description provided.

@sebsheep
Copy link
Contributor Author

Hi @mdgriffith , what do you think about this optimization? Maybe it was unclear from my initial PR without message, I just would like to talk about this before going through implementation.

@gampleman
Copy link

I wonder if minifiers don't already do this transformation?

@mdgriffith
Copy link
Owner

Hey! Sorry, I missed the initial notification for this PR.

Interesting, yeah I think it could be cool to explore.

First thing would be to see if it has an effect or if minifiers or even v8 are smart enough to fix the situation automatically. I'd recommend putting together an elm-benchmark and adjusting the js by hand to see if you can see a benefit. Maybe try it before and after a minifier too so we have an idea if that changes it.

If you have those results, we can add them to the description you've submitted here. This project is not only a transformer, but also a place to aggregate knowledge like this.

That can inform if it makes sense to do an implementation.

Thanks for the write up!

@sebsheep
Copy link
Contributor Author

Yeah, will try to do that. @mdgriffith do you have any example of elm-benchmark running modified JS?

@mdgriffith
Copy link
Owner

Hm, so I was thinking on this a little bit. Here are the questions that I think about:

  1. Does V8 or a minifier like terser handle this optimization automatically. (My guess is no, but curious to know the answer!)
  2. What sort of overhead is actually present.

So to test these we probably don't even need to modify the JS. I'd just set up a benchmark like the following:

module Main exposing ( main )

import Benchmark exposing (Benchmark)
import Benchmark.Runner exposing (BenchmarkProgram, program)

type Wrapper = Wrapper Int

toInt : Wrapper -> Int
toInt (Wrapper i) = i

range =
    List.range 0 1000 

wrapped =
     range
         |> List.map Wrapper 

bench : Benchmark
bench =
    Benchmark.describe "List.map unwrap"
        [ Benchmark.benchmark "elm" (\_ -> List.map toInt wrapped |> List.sum)
        , Benchmark.benchmark "without wrapper" (\_ -> range |> List.sum)
        ]


main : BenchmarkProgram
main =
    program <|
        Benchmark.describe "Unwrapping overhead"
            [ bench
            ]

Then I'd compile with --optimize, run it, then copy the JS, run it through terser and run that. Probably makes sense to double check both chrome and firefox.

I'm also curious about

  1. What situations this commonly shows up in? I definitely get the theoretical situation, though in general I try to look at realworld projects as much as possible. I wonder if we could ask in elm-webgl or gamedev on slack to see if anyone has a realworld piece of code that relies a lot on the single variant unwrapping.
  2. I'm also thinking how about how hard this transformation would be in the code. I think it might get pretty complex :/

@sebsheep
Copy link
Contributor Author

sebsheep commented Apr 18, 2021

I've run the benchmark you've proposed, and neither Terser nor V8 perform this "identity simplification" (V8 is even a bit less performant than FF with the wrapped one). Minifier doesn't seem to change anything.

I paste my results here for the reccord

Non minified

Browser Kind Runs/sec Fit
FF With wrapper 13,372 99.84
Without wrapper 64,103 99.78
Chrome With wrapper 10,034 99.91
Without wrapper 374,993 99.90

(I suspect V8 to recognize the sum as a special built-in function)

Minified

Browser Kind Runs/sec Fit
FF With wrapper 13,955 99.84
Without wrapper 63,292 99.88
Chrome With wrapper 9,694 99.96
Without wrapper 378,327 99.98

@sebsheep
Copy link
Contributor Author

  1. What situations this commonly shows up in? I definitely get the theoretical situation, though in general I try to look at realworld projects as much as possible. I wonder if we could ask in elm-webgl or gamedev on slack to see if anyone has a realworld piece of code that relies a lot on the single variant unwrapping.

I often use this opaque type technique to have some kind of guarantees on the value I manipulate (like wrapping a string for an email). This is also probably used in some computation intensive games -- or maybe people don't use that because of this performance penalty. I'll ask in the gamedev & webgl channels.

  1. I'm also thinking how about how hard this transformation would be in the code. I think it might get pretty complex :/

Yeah, I'm a bit worried about this as well. Maybe we could get have a useful but not optimal transformation only looking at List.map fn cases where fn is equivalent to identity.

@sebsheep
Copy link
Contributor Author

sebsheep commented Apr 18, 2021

Interesting answer from Ian MacKenzie:

I have definitely hit this, there are some really cool things I think I could do with elm-geometry and related packages if List.map somethingEquivalentToIdentity someList was turned into a no-op
Especially around coordinate system transformations
So yes, if that was an optimization that elm-optimize-level-2 could perform that could be extremely useful
Although of course I'd want to make sure that performance was still reasonable for anyone not using elm-optimize-level-2
Would it be possible to do the same thing for Array at the same time? Less critical but would be a nice to have

@mdgriffith
Copy link
Owner

Interesting! That's great to know.

Yeah, it's a little tricky because if Ian or Andrey or someone starts counting on this optimization and uses it significantly, then it's likely that that will lower performance for people who don't use elm-optimize-level-2 🤔 Not quite sure what to do about that. It's possibly an argument not to include the transformation in the tool.

Ignoring that for the moment, it seems there are probably three situations of interest:

  1. List.map unwrap
  2. Array.map unwrap
  3. Replacing individual instances of unwrap with the value that's being passed in.

@gampleman
Copy link

I think this technique is very common because the elm compiler already promises half of the optimisation: it removes the runtime overhead of the type wrapper, it just doesn't inline what remains. So in some sense having a general inliner pass would help with this - but I guess with asset size being so important for web apps aggressive inlining is a tricky technique to get right.

@sebsheep
Copy link
Contributor Author

Thoughts about this transformation: doing this in the JS compiled code seems a bit complicated. @mdgriffith What would you think about "pretransform" the elm code before passing it to the compiler? My idea is to transform:

type Wrapper = Wrapper Int

into

type alias Wrapper = Int

replace all the "wrap/unwrap" functions with identity and then run https://github.com/jfmengels/elm-review-simplify to remove all the List.map identity. Thus I can reuse the work from @jfmengels and have more control on what I'm doing (this kind of transformation ask to have a pretty precise knowledge of the source code, then it makes more sense to me to perform it at the "elm" level).

Overall the process would be:

elm make --output /dev/null # check for error
elm-review unwrap-custom-type # the rule I'd have to implement
elm-review simplify # Jeroen's rule
elm make --optimize 
jsOptimizer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants