Drop unsafe from PQconsumeInput FFI import#9
Merged
phadej merged 1 commit intohaskellari:masterfrom Jul 29, 2020
Merged
Conversation
Some paths in PQconsumeInput can go into blocking I/O calls. This has really bad interactions with GC; the RTS ends up waiting on IO (potentially, network) before being able to start Garbage Collection -- meanwhile, any Haskell mutation is blocked all this time. See the related research in: https://gitlab.haskell.org/ghc/ghc/-/issues/18415 Regardless of further issues in GHC RTS -- this call *must not* be marked "unsafe".
Collaborator
|
This makes sense. Potentially blocking / "slow" FFI calls shouldn't be
Please me ping me again, if I don't act during July. |
Contributor
Author
|
Hey @phadej ping :) I know there're conferences and such... Pinging as you requested. I did already check all the bindings (what they do on libpq-side) — and my judgment is, that they can be mostly left with You can of course go over it once again, and recheck yourself. I'd just like to land this PR sooner rather than later :) |
Collaborator
|
I'll make release hopefully soon. Have to check on Windows with newer Win32 version, i.e .adopt to upcoming GHC-9.0 release. |
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @phadej!
I've been doing performance testing of PostgREST (which indirectly uses this library) with GHC 8.10.1 (and 8.8.3 too). We have a latency spikes issue with it, easily attributable to idle GC pauses. That is, tens-to-hundreds-of-milliseconds long stop-the-haskell-world episodes when the GC is not using the CPU; is idle; blocked.
You can check out my methods & intermediate results in this thread: https://gitlab.haskell.org/ghc/ghc/-/issues/18415
While I currently cannot completely explain all the idle pauses — it's 100% clear that one of contributing issues is this line here in
postgresql-libpq. As the commit message says:PQconsumeInput()in libpq.so can go into blocking IO. While the GHC User Guide isn't terribly clear on how FFI interacts with GC, you can see this doc patch and this graph. All together, this leads to the conclusion that the import ofPQconsumeInputshould not be markedunsafe.Sure thing, I did test this change (and not only it). It's a bit hard to summarize concisely — but if you let me hand-pick a few graphs, I'd say that this one is "typical" for the code in
master:The vertical axis is latency of PostgREST handling HTTP requests; red lines are 10s timeouts. 1000 requests-per-second load; everything run on a modest SSD laptop, without CPU saturation. Here's a similar graph for a build with this patch applied:
(Well actually, all
postgresql-libpqimports are unmarked unsafe in the latter graph; but onlyPQconsumeInputmatters.)Might not seem significant (and I can pick way more dramatic graphs) — but when I drill down into
threadscopeprofiles, this patch does eliminate places like this.Please review, merge & release.