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

settable stackdepth via pd message #486

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

danomatika
Copy link
Contributor

This PR adds a settable max stack depth before throwing a stack overflow error. A new "pd stackdepth" message can set the value with a float > 0 or print the current value to the console.

This is an implementation of #482.

One thing missing is a help patch for the manual explaining how/why you would need this.

Screenshot with stackdepth at 1000:

screen shot 2018-09-22 at 1 55 43 am

and stackdepth at 10000:

screen shot 2018-09-22 at 1 55 50 am

@danomatika danomatika added the feature suggestion for an enhancement label Sep 22, 2018
@HenriAugusto
Copy link
Contributor

awesome!

One thing missing is a help patch for the manual explaining how/why you would need this.

I've had a fast Fibonacci algorithm lying around so i put together a little help patch.

recursionhelp

recursionFibonacciExample.zip

It's just a sketch but maybe something along these lines.

Copy link
Contributor

@umlaeute umlaeute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.
and thanks for choosing stackdepth over stackiter :-)

@danomatika
Copy link
Contributor Author

@umlaeute Where do you think a good place for the patch would be? doc/2.control.examples/, doc/7.stuff/?

@claudeha
Copy link
Contributor

Note that setting it too high (I tried 2e9) may cause hard crashes instead of soft loop-breaking, dependant on OS executable stack limits and how much each object in the chain puts on the stack:

$ ulimit -s
8192
$ pd -open stackoverflow.pd
Segmentation fault

simple OS stack overflow, increasing the limit shows problems too:

$ ulimit -s unlimited
$ pd -open stackoverflow.pd
...
max size for a Tcl value (2147483647 bytes) exceeded
Aborted
...
Program received signal SIGPIPE, Broken pipe.
(gdb) bt
#0  0x0000155554dd097a in __libc_send (fd=34, buf=0x1554cb1c5010, 
   len=len@entry=18446744071562080225, flags=flags@entry=0)
at ../sysdeps/unix/sysv/linux/send.c:28
#1  0x00005555556182fb in sys_trytogetmoreguibuf (newsize=<optimized out>)
   at s_inter.c:662
...
(pd by this point is using almost 16GB RAM)

I think it's a useful feature, but maybe should come with a warning like [until] does.

Perhaps actually measuring how much OS stack space was used / is left would be an alternative approach?

@danomatika
Copy link
Contributor Author

Perhaps actually measuring how much OS stack space was used / is left would be an alternative approach?

For something like this, I say "use with caution" as it's more of an edge case.

@danomatika
Copy link
Contributor Author

I've adapted the Fibonacci example by @HenriAugusto as a new control example for recursion. The stack overflow & stack depth info are also here, including the warning by @claudeha.

@danomatika
Copy link
Contributor Author

doc/2.control.examples/25.recursion.pd looks like this:

Screen Shot 2019-03-16 at 3 04 50 PM

@danomatika
Copy link
Contributor Author

danomatika commented Mar 16, 2019 via email

@porres
Copy link
Contributor

porres commented Mar 16, 2019

yeah, you need "0.50-0" or something (but I guess you know it better than me)

@millerpuckette
Copy link
Contributor

millerpuckette commented Mar 16, 2019 via email

@danomatika
Copy link
Contributor Author

I use "updated for version 0.50." (a period after the number, which makes it
a symbol).

Would it make sense to treat the comment content atoms as symbols only?

@millerpuckette
Copy link
Contributor

millerpuckette commented Mar 16, 2019 via email

@jonwwilkes
Copy link

Setting the Pd stack size to 10000 as you do in your documentation is equivalent to turning off Pd's stack overflow crash protection under Windows. (Tested on Windows 7, but it's likely the same under Windows 10.)

Test it with the simple case of infinite recursion with [float 42]x[t b] (with the output of trigger feeding into the left inlet of [float]). At stack size of 10,000 you'll blow the Windows stack and crash Pd before Pd can break the loop and output a "stack overflow" error. (And I just happened to test on Windows, I have no idea about other OSes or even different distros of Linux/BSDs.)

This means that even an abstraction developer who fiddles with this stackdepth method could accidentally limit their library to only work on a subset of Pd's supported platforms[1]. Given the number of users I've seen with spaghetti patches that generate stack overflows and still somehow output realtime audio, such a nested abstraction would turn a runtime nuisance into a showstopper. (And keep in mind that blowing the stack doesn't just crash the language interpreter but also the GUI. This would be like a Python bug that somehow also guaranteed to take down the user's IDE.)

If there's truly a need for the larger limit please hide it behind a startup flag. At least then a library developer will come to the list to ask about portability before shipping a footgun that may not even require this method. (E.g., list-abs/list-drip does not depend on it.)

[1] Even if one memorized the default OS stack sizes for all supported Pd platforms, I'm not sure one could accurately estimate stack sizes just by looking at the number of objects involved in the chain of recursion. For example, the list processing macros will use alloca for smaller list sizes. So it's possible a user could actually test for crashes on every single platform but choose a test list size large enough to trigger heap allocation instead of stack allocation. The whole point of abstractions is abstracting away implementation details like that.

@Spacechild1
Copy link
Contributor

I kind of agree that increasing the stack limit is rather dangerous. As Jonathan correctly notes, Windows usually has a much smaller stack size than macOS and Linux. The default stack size on the main thread is 1 MB and it is set by the linker (you can increase it with a linker flag). I can imagine that people would start developing large recursive algorithms which might work fine on macOS/Linux but crash on Windows.

For example, the list processing macros will use alloca for smaller list sizes. So it's possible a user could actually test for crashes on every single platform but choose a test list size large enough to trigger heap allocation instead of stack allocation.

that's also an important point!

Generally, everything you can do with recursion you can also do with iteration and in Pd you should prefer the latter.

@danomatika
Copy link
Contributor Author

danomatika commented May 30, 2019

This PR was really just an answer to (what seemed like) a simple request. I have no major skin in it. In doing this initial work, it did not occur to me that abstraction and library authors might use/abuse it.

I see 3 options for this:

  1. allow changing the stack limit via a message and consider deeper recursion officially supported with the example patch, this would require increasing the windows stack size as Christof notes
  2. only allow chancing the stack limit via a startup flag as Jonathan suggests and keep the setting only for advanced usage
  3. keep things as they are, do not integrate this PR, and shallow recursion works but is not officially supported

@millerpuckette
Copy link
Contributor

There's probably a way to set the true stack size in Windows (and also in Linux or Mac) - if anyone wants to go through the pain of figuring out how to just declare a fized, compile-time-known stack size, that would be the ideal. (It looks like this is configurable in freeRTOS and defaults to about 3Kbytes... so having a compile-time-configurable constant, even if it's just a guess, might be a good option.

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

Successfully merging this pull request may close these issues.

None yet

8 participants