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

debug_backtrace() assertions should be disabled by default #217

Closed
Bilge opened this issue Apr 18, 2018 · 3 comments
Closed

debug_backtrace() assertions should be disabled by default #217

Bilge opened this issue Apr 18, 2018 · 3 comments
Assignees
Labels

Comments

@Bilge
Copy link
Contributor

Bilge commented Apr 18, 2018

I just learned, the hard way, that debug_backtrace() is called by default in Amp. After hitting consistent memory limit errors, I went through the painful process of memory debugging PHP - a process I wouldn't wish on my worst enemy - to figure out what was going wrong, and to my dismay, discovered massive memory allocation was happening in innocuous-looking assertions throughout Amp, particularly in Placeholder and Deferred. Turning this feature off by setting AMP_DEBUG=0 was enough to fix the issue, but to save someone else having to go through similar pain, I suggest it should be off by default.

Perhaps someone was thinking that it would be sufficient to recommend assertions be disabled if you don't want this feature, but in case that is what you're thinking, I'm going to pre-empt that by suggesting that would be wrong because a developer may wish to use assertions in their own code without debugging Amp.

Calling debug_backtrace() is not a sane default because it was causing memory usage to balloon to roughly double in my application, resulting in fatal errors.

@trowski
Copy link
Member

trowski commented Apr 18, 2018

Storing the backtrace of resolved promises is not meant for debugging Amp, but rather for debugging your code. If you double-resolve a promise, it is useful to know the first place that promise was resolved.

If your application is running out of memory due to these backtraces, that indicates to me that you're holding onto too many resolved promises. Look for where data may be being read faster than it is being consumed. From your messages in IRC, I suspect you are emitting data in a Producer faster than it is consumed, leading to many emitted values (and promises) being held in memory. Consider yielding the promise returned from the emit callback to only read values as fast as they are consumed.

@Bilge
Copy link
Contributor Author

Bilge commented Apr 18, 2018

If your application is running out of memory due to these backtraces, that indicates to me that you're holding onto too many resolved promises.

I'm not aware of anywhere I might be doing that, but if that was the case, wouldn't my memory be leaking? It was completely stable (+/- 3MB), neither growing nor shrinking, for 50 minutes straight whilst constantly downloading data (after disabling AMP_DEBUG).

I suspect you are emitting data in a Producer faster than it is consumed

You are exactly correct. However, due to my Throttle, it cannot resolve more than the max concurrent limit (currently 30), and I am monitoring this to ensure it is working correctly.

@trowski
Copy link
Member

trowski commented Apr 18, 2018


Promises without the backtrace have a small memory footprint, so it's possible that without storing the backtraces that it is not consuming enough memory to be a problem.

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

No branches or pull requests

3 participants