-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Increase idle GC interval to workaround CPU leak issue #2581
Conversation
Deploy preview for hasura-docs ready! Built with commit 7c9b300 |
Review app for commit 3d0c16f deployed to Heroku: https://hge-ci-pull-2581.herokuapp.com |
Is there any reason why we can't disable idle gc completely? |
@0x777 The reason not to (which I forgot when I responded to the ticket at first) is idle GC is necessary to ensure finalizers are run promptly and deadlocked threads are cleaned up promptly. It could result in some weird behavior or resource leakage with idle GC disabled. It's just hard to anticipate what might go wrong, e.g. what libraries might rely on finalizers running somewhat promptly in all cases. It could be fine but this is more cautious and seems to fix things. |
Review app for commit 7f8fd0c deployed to Heroku: https://hge-ci-pull-2581.herokuapp.com |
I scaled up the timing intervals for polling threads by the same degree as the proposed fix here (0.3 sec -> 2 sec), and the fix seems to still work. So hopefully the |
@lexi-lambda if you could review when you have the chance ^ :) |
Just asking: is there an open GHC issue about this? |
@lexi-lambda Nothing I've found where anyone suggests GHC is probably doing something broken. Thanks for asking, a little survey: The two old tickets I was familiar with and linked elsewhere A confusing writeup that might have good information but I didn't try to parse: Two newish tickets that might be exhibiting the issue I wasn't inclined to open a new issue without a small test case, and wasn't real confident creating one would be easy. |
Makes sense. This comment from Simon Marlow is interesting:
That seems like it could apply to us, but on its own, it doesn’t get us much further to understanding why that would trigger anything pathological. In any case, this workaround seems harmless. |
@lexi-lambda I think you may have to press the button yourself (I'm not authorized to merge, not sure if that's the intention) :-) |
I think @0x777 is actually still the only person who can push the button, even though I’ve been marked as the code owner. I think he can also fix that, though. |
This seems to resolve the issue locally (and has worked in the past), but it's not clear what exactly is going on here (in particular, why this should resolve what looks like a memory leak). It certainly seems like a GHC issue of some sort. Closes hasura#2565
Review app https://hge-ci-pull-2581.herokuapp.com is deleted |
Review app for commit 7c9b300 deployed to Heroku: https://hge-ci-pull-2581.herokuapp.com |
Thanks for the PR. Does this mean we wont have to use |
@elitan that's the idea, but if you can give it a try and confirm it solves the issue for your deployment that would be really helpful! |
@jberryman Will do! |
This seems to resolve the issue locally (and has worked in the past), but it's not clear what exactly is going on here (in particular, why this should resolve what looks like a memory leak). It certainly seems like a GHC issue of some sort. Closes hasura#2565
This seems to resolve the issue locally (and has worked in the past),
but it's not clear what exactly is going on here (in particular, why
this should resolve what looks like a memory leak). It certainly seems
like a GHC issue of some sort.
Closes #2565