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 evaluation order for overapplications when using Closure #967

Merged
merged 4 commits into from Dec 29, 2016

Conversation

Projects
None yet
5 participants
@mshinwell
Contributor

mshinwell commented Dec 13, 2016

When using the Closure pass (i.e. non-Flambda) the evaluation order for overapplications does not match the bytecode compiler and Flambda: as shown by PR#6136, the subset of arguments forming the full application are evaluated first, then the function is called; and then the remaining arguments are evaluated. This patch fixes that by let-binding the arguments before the call.

@mshinwell mshinwell added the bug label Dec 13, 2016

@mshinwell mshinwell added this to the 4.05.0 milestone Dec 13, 2016

@mshinwell mshinwell requested a review from xavierleroy Dec 13, 2016

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 13, 2016

Contributor

This patch fixes that by let-binding the arguments before the call.

It would be useful to see the impact on performance, even on some micro-benchmarks only. Evaluating extra arguments before the call is likely to create more transit with the memory.

Contributor

alainfrisch commented Dec 13, 2016

This patch fixes that by let-binding the arguments before the call.

It would be useful to see the impact on performance, even on some micro-benchmarks only. Evaluating extra arguments before the call is likely to create more transit with the memory.

Show outdated Hide outdated Changes
@@ -185,6 +185,9 @@ Next version (4.05.0):
by zero.
(Jeremy Yallop)
- GPR#967, PR#6536: Fix Closure so that overapplication evaluation order
matches the bytecode compiler and Flambda.

This comment has been minimized.

@yallop

yallop Dec 13, 2016

Member

The Mantis PR number should be 6136, not 6536. It's also conventional to list the author and the reporter.

@yallop

yallop Dec 13, 2016

Member

The Mantis PR number should be 6136, not 6536. It's also conventional to list the author and the reporter.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Dec 18, 2016

Contributor

Over-applications are exceedingly rare in practice, so I'm not too worried about a performance degradation here.

Contributor

xavierleroy commented Dec 18, 2016

Over-applications are exceedingly rare in practice, so I'm not too worried about a performance degradation here.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 19, 2016

Contributor

I also think the rarity of these cases means that we shouldn't really be worrying about the performance impact of this change. Furthermore, constructing benchmarks that actually give meaningful results for changes like this probably isn't entirely straightforward, and given limited time I think our attention is best concentrated elsewhere (e.g. #966).

Would anyone like to comment on the correctness of this patch?

Contributor

mshinwell commented Dec 19, 2016

I also think the rarity of these cases means that we shouldn't really be worrying about the performance impact of this change. Furthermore, constructing benchmarks that actually give meaningful results for changes like this probably isn't entirely straightforward, and given limited time I think our attention is best concentrated elsewhere (e.g. #966).

Would anyone like to comment on the correctness of this patch?

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 27, 2016

Contributor

@let-def I nominate you to review. :)

Contributor

mshinwell commented Dec 27, 2016

@let-def I nominate you to review. :)

@let-def

This comment has been minimized.

Show comment
Hide comment
@let-def

let-def Dec 28, 2016

Contributor

This looks good to me.

I was wondering if there was an interest in forcing evaluation order of all arguments instead of only over-applied ones (folding only on rem_args), but I don't think this change anything in practice.

Contributor

let-def commented Dec 28, 2016

This looks good to me.

I was wondering if there was an interest in forcing evaluation order of all arguments instead of only over-applied ones (folding only on rem_args), but I don't think this change anything in practice.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 28, 2016

Contributor

Modulo any problems in the backend, of course, but we're addressing those in a different GPR. Thanks for the review, I will merge this.

Contributor

mshinwell commented Dec 28, 2016

Modulo any problems in the backend, of course, but we're addressing those in a different GPR. Thanks for the review, I will merge this.

@mshinwell mshinwell added the approved label Dec 28, 2016

@mshinwell mshinwell merged commit 8127e31 into ocaml:trunk Dec 29, 2016

2 checks passed

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

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

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