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

Excessive stack usage in FLambda middle end. #7842

Open
vicuna opened this issue Aug 28, 2018 · 18 comments
Open

Excessive stack usage in FLambda middle end. #7842

vicuna opened this issue Aug 28, 2018 · 18 comments

Comments

@vicuna
Copy link

vicuna commented Aug 28, 2018

Original bug ID: 7842
Reporter: @ejgallego
Assigned to: @chambart
Status: assigned (set by @chambart on 2018-08-31T23:34:23Z)
Resolution: open
Priority: low
Severity: minor
Platform: Linux
OS: Ubuntu
OS Version: 18.04
Version: 4.07.0
Category: middle end (typedtree to clambda)

Bug description

Dear OCaml devs, Jason Gross has reported a case where flambda-enabled ocamlopt stacks overflow whereas the normal version compiles fine.

Increasing the stack limit solves the issue, so I am unsure if you would even consider this a bug, but reporting anyways.

Original Coq issue: coq/coq#8313

Steps to reproduce

Download the file: https://github.com/coq/coq/files/2328849/unsaturated_solinas.tar.gz

Then compile with 4.07.0+flambda. The stack trace is:

Fatal error: exception Stack overflow
Raised by primitive operation at file "middle_end/flambda_iterators.ml", line 739, characters 25-38
Called from file "middle_end/flambda_iterators.ml", line 739, characters 25-38

@vicuna
Copy link
Author

vicuna commented Sep 10, 2018

Comment author: @chambart

The particular part of flambda that is failing is not tail recursive due to an optimization to limit allocations. It is possible to mitigate this without allocating too much, but this would be quite tedious. Would you consider that critical enough to dedicate more time to it ?

@vicuna
Copy link
Author

vicuna commented Sep 10, 2018

Comment author: @ejgallego

I would dare to say that this is very low priority. Thanks for looking into it.

@Chris00
Copy link
Member

Chris00 commented Sep 25, 2019

Compiling Color_brewery with flambda also triggers this problem. (I have not investigated how to circumvent the problem to make this package installable on flambda variants but this is annoying.)

@ejgallego
Copy link

@Chris00 using ulimit -s STACK_SIZE on Linux should solve the issue, where the stack size will of course depend on your program.

ejgallego added a commit to ejgallego/coq that referenced this issue Mar 10, 2020
ejgallego added a commit to ejgallego/coq that referenced this issue Mar 10, 2020
ejgallego added a commit to ejgallego/coq that referenced this issue Mar 10, 2020
@ejgallego
Copy link

We've hit this bug again, and this time looks more serious. The following "innocuous" commit makes the compilation of Coq to require 4x the stack size:

"[parsing] Make grammar rules private."
coq/coq@4750e5e
from coq/coq#11647

What used to work with 2k stack, now it needs close to 16k :( Note that the build uses -O3 -unbox-closures

Overflow in in asmspill; given the changes the behavior here is certainly very weird...

Tested on flambda - 4.07 -- 4.09

@SkySkimmer
Copy link

The following "innocuous" commit makes the compilation of Coq to require 4x the stack size:

Do any specific file(s) need the extra stack?

@ejgallego
Copy link

ejgallego commented Mar 10, 2020

Do any specific file(s) need the extra stack?

The stack overflow is in g_vernac.ml , which is not touched by that commit, only its API.

Likely the inlining profile changes [indeed it is a good thing flambda is going deep on that file]

@ejgallego
Copy link

This is becoming more and more common, for example coq/coq#12036 , even worse, for some files not even increasing the stack size will work [unless ridicoulous values are used]

In practice we are losing more time that the amount that would have taken to convert map_exprs_at_toplevel_of_program to be tail-rec; I may give it a go.

ejgallego added a commit to ejgallego/coq that referenced this issue Apr 8, 2020
As of today flambda is pretty stack-hungry for some developments, in
particular it doesn't work [`StackOverflow` in fiat-crypto extracted
code even with large stacks.

We are thus forced to revert fiat-crypto's compilation to the regular
OCaml compiler.

This is OCaml bug ocaml/ocaml#7842
ejgallego added a commit to ejgallego/coq that referenced this issue Apr 8, 2020
As of today flambda is pretty stack-hungry for some developments, in
particular it doesn't work [`StackOverflow` in fiat-crypto extracted
code even with large stacks.

We are thus forced to revert fiat-crypto's compilation to the regular
OCaml compiler.

This is OCaml bug ocaml/ocaml#7842
@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Apr 12, 2021
@ejgallego
Copy link

Still an issue.

@github-actions github-actions bot removed the Stale label Apr 14, 2021
@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Apr 15, 2022
@gasche gasche removed the Stale label Apr 15, 2022
@lthls
Copy link
Contributor

lthls commented Apr 15, 2022

Looking at the history, there seem to be a number of separate issues (that all manifest as a Stack_overflow). The first report, about a stack overflow within Flambda itself, is still valid. The second report (about Color_brewery), I can't reproduce on 4.13. For the other issues, it's not clear how I could try to reproduce them; given that one of them mentions asmspill, it's quite possible that these are different issues.

@ejgallego
Copy link

@lthls we have indeed quite a few examples where flambda needs a lot of stack, granted these are not very natural examples tho, as they arise mostly from Coq-generated code.

@lthls
Copy link
Contributor

lthls commented Apr 15, 2022

I'm expecting that some of these examples (like the original one) are cases where the Flambda pass itself needs a lot of stack, but others are likely caused by other parts of the backend needing a lot of stack when processing the output of Flambda. If that's the case, making Flambda tail-recursive won't help, and it would be useful to know whether you're encountering those issues too.
For example, with Flambda 2 (which is mostly tail-recursive), we still have to add the -linscan flag when we compile Coq to avoid stack and memory blowups later in the backend.

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Apr 17, 2023
@ejgallego
Copy link

Still an issue.

@github-actions github-actions bot removed the Stale label Apr 19, 2023
@lthls
Copy link
Contributor

lthls commented Apr 19, 2023

Still an issue.

Do you know if it's still an issue with 5.0 ? If so, it's not a regular stack overflow, as the default stack size is much bigger in 5.0.

@ejgallego
Copy link

Will test, thanks for the suggestion!

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

No branches or pull requests

8 participants