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

Fix bad interaction between Simplif and optimized compilation of tuple binding #1497

Merged
merged 2 commits into from Dec 1, 2017

Conversation

Projects
None yet
3 participants
@alainfrisch
Copy link
Contributor

alainfrisch commented Nov 29, 2017

Fix MPR#7680.

The proposed fix is to make the simplification of "exits" (in Simplif) more robust against static raises that "traverse" a try...with. The simplification consists in inlining the static handler when there is a single corresponding static raise (in tail context). The fix disables this inlining if the static handler and static raise are at different nesting level w.r.t. try...with (the level is incremented when inspecting the body of the try...with).

@lthls

This comment has been minimized.

Copy link
Contributor

lthls commented Nov 29, 2017

Unless I'm reading your patch incorrectly, you are never updating the exits hash table, and thus sharing the default empty_exit with all exits, which essentially disables the optimization for any non trivial program.
I'm surprised there's nothing in the testsuite that breaks, but otherwise it looks sound (both in the current form and with the necessary fix).

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

alainfrisch commented Nov 29, 2017

Ah, yes, thanks! Moreover, I was updating the global empty_exit record...

I've pushed a hopefully fixed version.

@lthls
Copy link
Contributor

lthls left a comment

I've made a few comments that you might want to think about, otherwise this patch looks correct both in preventing the reported bug and preserving the desired optimizations.

if count = 0 then
(* Discard staticcatch: not matching exit *)
simplif l1
else if i >= 0 && count = 1 && max_depth <= !try_depth then begin

This comment has been minimized.

@lthls

lthls Nov 29, 2017

Contributor

As a side note, I think you've made the negative index trick irrelevant; you could probably remove the test i >= 0 and even remove Lambda.next_negative_raise_count.
Also, max_depth < !try_depth would be very worrying; I'd prefer an assert false for this case or at least an equality check instead.

This comment has been minimized.

@alainfrisch

alainfrisch Nov 29, 2017

Author Contributor

negative index trick irrelevant

Indeed. I thought it was also used to prevent inlining the handler when the static raise is not in tail context, but apparently this cannot happen.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

alainfrisch commented Nov 29, 2017

Comments from @lthls taken into account.

Since this touches the compiler, I need a formal review from a fellow maintainer before merging the PR. @gasche perhaps?

@alainfrisch alainfrisch added this to the 4.07 milestone Nov 29, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 30, 2017

Before I spend the time to review this, I would like to ask whether this is the right direction. My understanding is that @lthls is working on a better way to handle the interaction between static and dynamic raises anyway, that may require changing what is in the proposed patch and, in any case, give us tools to make it nicer/simpler. In that context, isn't it more reasonable to just disable the optimization when hitting a trywith, as Alain first suggested?

P.S.: If @lthls is willing to take the responsibility to formally "approve" this (as in: to feel bad is it turns out to be broken in some way), I would be happy to merge without doing a review myself -- feel free to use the github "review > approve" interface to mark this, but please confirm in some way in any case. Otherwise I need to do a review, and see remark above.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

alainfrisch commented Nov 30, 2017

We are talking about a silent mis-compilation, which is rather critical. I'd prefer not delay the fix, which is relatively simple, because of a possibly coming full overhaul you are referring to. I guess this is #1482 -- which does not seem fully consensual, so I wouldn't not bet on it being ready and merged in time for 4.07.

Cherry-picking for 4.06.1 should also be considered.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 30, 2017

From the point of view of correctness, and in particular if we are discussing merging in the maintenance branch, the patch you propose now is sensibly more complex than just disabling the optimization in trywith (I don't think that there is much code out there relying on this edge-case). Is it a good use of our scarce reviewer resources?

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

alainfrisch commented Nov 30, 2017

Disabling the optimization on try...with might lead to serious performance degradation, which is another form of regression (less critical than most, but harder to detect and predict).

To avoid wasting scarce reviewer resources, we can do nothing now and wait for the feature freeze for 4.07 and decide at that date to review or not this current PR depending on whether #1482 has been merged or not. I believe this will increase the pressure on the release process, which is actually more critical than the net amount of review ressources in my opinion.

@alainfrisch alainfrisch added the bug label Nov 30, 2017

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

alainfrisch commented Nov 30, 2017

If @lthls is willing to take the responsibility to formally "approve" this (as in: to feel bad is it turns out to be broken in some way), I would be happy to merge without doing a review myself -- feel free to use the github "review > approve" interface to mark this, but please confirm in some way in any case.

@lthls Are you willing to assume this honor and responsibility? 😄

@lthls

This comment has been minimized.

Copy link
Contributor

lthls commented Nov 30, 2017

I had to look at the related code recently so I'm convinced that the patch is correct, and if it breaks something I'd definitely want to know about it.
And #1482 is not aiming for 4.06.1, so I'd approve this as a bug fix and I'll remove it as redundant on my pull request when I rebase it.

@lthls

lthls approved these changes Nov 30, 2017

Copy link
Contributor

lthls left a comment

And here is the official seal of approval.

@gasche

gasche approved these changes Nov 30, 2017

Copy link
Member

gasche left a comment

(approved based on @lthls's approval, I haven't done a review myself.)

Thanks!

@alainfrisch, feel free to merge. It may be appropriate to squash your commits a bit, and right now there is a conflict, otherwise I would have merged myself.

@gasche gasche removed the review-needed label Dec 1, 2017

@alainfrisch alainfrisch force-pushed the fix_7680 branch from 2b347ce to 2de4daa Dec 1, 2017

@alainfrisch alainfrisch force-pushed the fix_7680 branch from 2de4daa to 0ee0448 Dec 1, 2017

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

alainfrisch commented Dec 1, 2017

Thanks, history rewritten as suggested. Waiting for CI to complete before merging.

@alainfrisch alainfrisch merged commit edc197b into trunk Dec 1, 2017

4 checks passed

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

@alainfrisch alainfrisch deleted the fix_7680 branch Jul 2, 2018

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.