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

Inline semigroupoidArr composition #1070

Merged
merged 7 commits into from Apr 23, 2015

Conversation

Projects
None yet
4 participants
@puffnfresh
Contributor

puffnfresh commented Apr 21, 2015

An example of the changes:

g :: Boolean
g = not <<< not $ true

f :: Boolean -> Boolean
f = not <<< not <<< not <<< not

f' x = (\y -> f y) x
var g = !!true;

var f = function (_44) {
    return !!!!_44;
};

var f$prime = function (x) {
    return f(x);
};

I did the following:

  1. Allowed everywhereOnJSTopDown to work in a Monad, default is Identity.
  2. Made untiFixedPoint work on a Monad.
  3. Added inlineAppliedVars optimisation, turning (\x -> f x) y into f y.
  4. Added inlineArrComposition optimisation, turning a >>> b into (\x -> a (b x)).

What are the drawbacks?

Should fix #1066.

puffnfresh added some commits Apr 21, 2015

Add everywhereOnJSTopDownM
Allows replacing JS nodes with ones calculated monadically.
Add inlineAppliedVars optimisation
Translates the following:

    (function(a) {
      return a * a * a;
    })(b)

Into:

    b * b * b
Add inlining of semigroupoidArr composition
The following program:

    f :: Boolean -> Boolean
    f = not <<< not <<< not

Is now compiled to:

    var f = function (_44) {
        return !!!_44;
    };
Combine function composition optimisations
They messed with each other when separate. Work really well combined.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 21, 2015

Coverage Status

Coverage increased (+0.04%) to 78.37% when pulling a411755 on puffnfresh:optimisation/inline-semigroupoid-arr into fa59cf1 on purescript:master.

coveralls commented Apr 21, 2015

Coverage Status

Coverage increased (+0.04%) to 78.37% when pulling a411755 on puffnfresh:optimisation/inline-semigroupoid-arr into fa59cf1 on purescript:master.

@garyb

This comment has been minimized.

Show comment
Hide comment
@garyb

garyb Apr 21, 2015

Member

Looks great to me, and good call on the Identity monad, I think we have some AST traversals we could have done that with.

Member

garyb commented Apr 21, 2015

Looks great to me, and good call on the Identity monad, I think we have some AST traversals we could have done that with.

@paf31

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Apr 21, 2015

Member

Looks good, thanks!

I'm always wary of inlining operations, since they have broken code in odd ways in the past. In this case, I thought the existing inlining code would have taken care of it, so I'd like to figure out why not:

https://github.com/purescript/purescript/blob/master/src/Language/PureScript/CodeGen/JS/Optimizer/Inliner.hs#L48

That code is very strict in terms of what it accepts, but only because so much code has broken in the past, and it's been changed a few times.

What is the set of transformations we want to enable, and can we figure out why the existing optimization isn't firing?

The fresh name code and <<< optimization look great. Should we optimize >>> too?

Member

paf31 commented Apr 21, 2015

Looks good, thanks!

I'm always wary of inlining operations, since they have broken code in odd ways in the past. In this case, I thought the existing inlining code would have taken care of it, so I'd like to figure out why not:

https://github.com/purescript/purescript/blob/master/src/Language/PureScript/CodeGen/JS/Optimizer/Inliner.hs#L48

That code is very strict in terms of what it accepts, but only because so much code has broken in the past, and it's been changed a few times.

What is the set of transformations we want to enable, and can we figure out why the existing optimization isn't firing?

The fresh name code and <<< optimization look great. Should we optimize >>> too?

Remove inlineAppliedVars
It seemed like this was necessary because I didn't fully understand all
that etaConvert was doing. It also looked like it was doing something
useful with the (<<<) optimisation but now removing it doesn't change
the behaviour.
@puffnfresh

This comment has been minimized.

Show comment
Hide comment
@puffnfresh

puffnfresh Apr 21, 2015

Contributor

@paf31 I removed inlineAppliedVars. At some point it seemed like it was making (<<<) work but now it doesn't seem necessary. I imagine the (<<<) optimisation just wasn't working properly at the time.

Contributor

puffnfresh commented Apr 21, 2015

@paf31 I removed inlineAppliedVars. At some point it seemed like it was making (<<<) work but now it doesn't seem necessary. I imagine the (<<<) optimisation just wasn't working properly at the time.

@paf31

This comment has been minimized.

Show comment
Hide comment
@paf31

paf31 Apr 21, 2015

Member

Looks good. I'll just wait for the CI build to finish.

Member

paf31 commented Apr 21, 2015

Looks good. I'll just wait for the CI build to finish.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 21, 2015

Coverage Status

Coverage increased (+0.03%) to 78.35% when pulling e929327 on puffnfresh:optimisation/inline-semigroupoid-arr into fa59cf1 on purescript:master.

coveralls commented Apr 21, 2015

Coverage Status

Coverage increased (+0.03%) to 78.35% when pulling e929327 on puffnfresh:optimisation/inline-semigroupoid-arr into fa59cf1 on purescript:master.

paf31 added a commit that referenced this pull request Apr 23, 2015

Merge pull request #1070 from puffnfresh/optimisation/inline-semigrou…
…poid-arr

Inline semigroupoidArr composition

@paf31 paf31 merged commit 0c81186 into purescript:master Apr 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment