Skip to content
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

Flambda: Add [Closure_origin.t] to trace inlined functions (merged) #1707

Merged
merged 6 commits into from Apr 10, 2018

Conversation

Projects
None yet
5 participants
@xclerc
Copy link
Contributor

xclerc commented Apr 9, 2018

see #1340.

(I have also re-read the diff after the merge to ensure the merge was correct.)

fyquah95 and others added some commits Sep 11, 2017

Flambda: Add [Closure_origin.t] to trace inlined functions.
This prevents infinite loop from inlining recursive functions passed via
CPS-style in -OClassic. (A standard recursive function would have been
caught by the [self_call] check).

@mshinwell mshinwell added the bug label Apr 9, 2018

@mshinwell mshinwell added this to the 4.07 milestone Apr 9, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 10, 2018

Approving based on approval from #1340 and @xclerc 's review of the merge diff.

@mshinwell mshinwell added the approved label Apr 10, 2018

@mshinwell mshinwell merged commit d88dbba into ocaml:trunk Apr 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

mshinwell added a commit that referenced this pull request Apr 10, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 10, 2018

Cherry-picked to 4.07 (91b14d5e71601e2e9aac5ebd046452526f6deb32).

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Sep 15, 2018

I believe that this change caused a regression in optimization of long composition sequences, see MPR#7849.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Sep 17, 2018

The 4.07 cherry-pick must be reverted ASAP. (The 4.07.1 release is imminent.)

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Sep 17, 2018

@xavierleroy I'm not sure anything needs doing here. This change was originally introduced because certain patterns of code were failing to compile at all (see #1340). It was cherry-picked onto 4.07 for that reason and is in the existing 4.07 release.

MPR#7849 is an example of code that should not actually have been optimised in the way it was being optimised prior to this patch. As such it seems reasonable for there to be an expected regression here. We have done work, to be presented at some point in the future, which improves the treatment of recursion in conjunction with inlining. This should optimise examples such as MPR#7849 robustly.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Sep 17, 2018

I got the timeline wrong and thought it was cherry-picked after 4.07.0 and for 4.07.1, which is not the case. Sorry about that. Nonetheless, I'm still puzzled as to why this bug fix degrades performance that much.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Sep 17, 2018

The reporter was relying on a "magically smart" optimization done by flambda before, I'm not terribly surprised that those are fragile. As far as I'm concerned, I'm satisfied with Leo's explanation here, that he has plans for things to improve again in the future.

(I would guess that identifying closure sites with not just their static source position, but also a fragment of the calling context (say, when the caller is inlined as well), would help get the best of both world, at the cost of course of a compilation time increase.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.