Repository navigation
safe vs. unsafe foreign imports #34
Description
Activity
this sounds like a valid/good idea! What should be the default is a more complex question, but that aside, both styles should be available!
Unsafe foreign imports which can block for unpredictable amounts of time cause performance problems that only emerge when scaling to multiple cores, because they delay the GC sync. This is a really annoying problem if it happens to you, because it's almost impossible to diagnose, and if it happens due to an unsafe call in a library then it's also really hard to fix.
So the safe versions should be the default, and if we have them at all, the unsafe versions should be named
unsafeFooor put in a separate.Unsafemodule, with clear documentation about the possible implications.Agreed, if the call would ever block (and that includes most filesystem functions) that means you want "safe".
Glad to see people agree :)
I'll try and prepare a patch then.
Even worse,
open(2)(which is used byopenFd) is markedunsafedespite the fact the it can block forever, eg when opening a named pipe withO_RDONLYand if no one has opened it for writing.Let me also point out that for many of the functions in
unix, the import actually happens inSystem.Posix.Internalsfrombase.So, question time. I've looked at the unix library code and also a bit at base (thanks @redneb for the hint). 'unix' alone has around 180 unsafe 'foreign imports'. We have a couple of ways to change things.
- Provide only safe variants for the imports that can block or take a long time. This would be simplest and cleanest solution, but has the drawback of not giving any choice to the user. It's debatable whether a choice is useful here, but who knows.
- Provide the safe and unsafe alternatives. Because in many cases, the C imports are not used as is, but instead are wrapped in a layer that makes them easier to use in Haskell, it means that we would have to either duplicate the code, or add a 'Bool' parameter that denotes the safe/unsafe version. This is ugly, so even if we only presented to the user the two functions (the safe and unsafe alternatives, partially applied), I don't quite like this solution. Actually implementing it would mean one of the following:
- separate "Unsafe" modules; this is unwieldy due to the number of modules that would be needd (we'd go from System.Posix.Files, System.Posix.Files.ByteString to Files, Files.Unsafe, Files.ByteString, Files.ByteString.Unsafe, urgh)
- functions in the same module, prefixed with
unsafe; this would be cleaner since at least all the code is in the same place, and keeps module count sane
- Provide a cabal flag to switch between the safe and unsafe alternatives; while this could work for
unix, I don't see it working forbase, so I just mention it as a possibility, but don't actually suggest it.
Given these options, I would propose going with the first one - no choice, but rather an informed switch from unsafe to safe, case-by-case. Howver, I've run some more benchmarks and, on the relatively slow laptop that I tested, criterion gives me:
- a "noop" C call (
void noop() { }) runs in around 20ns unsafe, and 240ns safe; that means, the worst slowdown is 12× - a
getpidcall, which is the most trivial after noop, is 26ns vs 250ns (<10×) - a
getenvcall, less trivial but still cheap, is 153ns vs 365ns (2.4×) - a
getgrgidcall, which is expensive and can block (when nsswitch contains remote service), takes (with local files only) 24.6µs vs 26µs, so only 5% slower
These kind of slowdowns range from close to noise to very significant, so I'm not sure anymore if we can afford not to give the option of switching, if we find a function call that is fast but could still block.
Thoughts on which way to go? So far I've narrowed down the list of potential "dangerous" ffi imports in 'unix' to around 80, mostly related to files, but not only. But still it's a lot of functions that would need to be duplicated…
I'd vote for providing
.UnSafemodules personally, if only becomes some of these system calls have different performance characteristics depending on the underlying unix system, so being overly prescriptive might be a bad idea.One other option:
- Default to safe for all the dangerous imports
- Provide a compile-time cabal flag to switch to unsafe
- Upload
unix-unsafeto Hackage which is the same asunixbut with the imports defaulting to unsafe.
@cartazio I'm against
.UnSafemodules, since as it was pointed out, this would blow up the hierarchy to contain stuff like.ByteString.UnSafe(or.UnSafe.ByteString);(on a related note, at some point we might want to add
ByteString-I/O based (i.e.ByteStringfor the payload) operations, that may bring yet another.ByteString...suffix to the hierachy)@simonmar the main problem I see with
unix-unsafepackage is that you'd have to depend onunix, as otherwise you'd end up with divergent types, if you need to pass values of types defined inunixto operations ofunix-unsafe; this may be quite a maintainance overhead of keepingunix-unsafein sync w/unixFwiw, I like the idea of a manual cabal flag to switch to the old
unsafesemantics, but defaulting everything that's dangerously blocking tosafeby default. I'd rather err on the safe-side and give people the option to turn off all safety-guards locally if they really want to. Let somebody else create anunix-unsafepackage if there's really demand for it.Fyi, as this isn't a new regression we'll probably not target GHC 7.10.1 for this fix
will making all the current calls use safe be part of 7.10?
@hvr: makes sense; also, we shouldn't hurry to fix this and introduce an API that is painful to correct later.
@cartazio: as I understand, no.The only thing I don't like about the cabal flag is that this will be a bit cumbersome to fix in "base" (which also has this kind of unsafe imports). I mean that I'd be surprised if anyone actually ever rebuilds base in their install, but maybe it does happen. On the other hand, the flag keeps the API clean and simple.
In any case, I'll write the cabal patch - it might take a bit to make sure I mark the correct imports safe; we'll see how it goes from there. Thanks for the feeback!
@cartazio while making all (potentially) blocking calls
safeis the morally-right-thing-to-do(tm), we have the problem of not knowing how bad the performance impact will be for real-world code. And it's a bit late in the release cycle for 7.10.1 to introduce such a risk, while OTOH we have had thoseunsafeFFI calls for ages and nobody complained till now...Maybe we can consider it for GHC 7.10.2 if we have some benchmark data for syscall-heavy real-world (test)programs by then...
22 remaining items
- What’s stopping us from moving the known problematic calls to safe? Their overhead will be on the order of a quarter microsecond. But that’s fine…On Tue, Nov 19, 2019 at 7:29 AM Niklas Hambüchen ***@***.***> wrote: @simonmar <https://github.com/simonmar> On https://gitlab.haskell.org/ghc/ghc/issues/13296#note_132146 you mentioned work on making safe syscalls faster (https://phabricator.haskell.org/D1466 - *RFC: foreign import ccall nonblocking*). Is this something that should still be done along with changes in here and GHC to address the performance reservations you had back then, or do you think we should switch unix (or more) to safe even if that work remains for later? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34?email_source=notifications&email_token=AAABBQSFBSGFWE6LXY3AZKLQUPLYZA5CNFSM4AZWIHN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEOAXOQ#issuecomment-555486138>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAABBQTCMQYB53LVXBQBXXLQUPLYZANCNFSM4AZWIHNQ> .
@cartazio - the conclusion back in 2015 was that the slowdown was 10× between safe and unsafe calls; for some workloads, this might not be a valid trade-off.
In any case, I'm attaching the WIP patch I had back then. The work is not very complex, but rather tedious - look at all call sites, and decide (by reading man pages and such) whether that's a safe, unsafe, or interruptible call, and annotate it as such.
@simonmar On https://gitlab.haskell.org/ghc/ghc/issues/13296#note_132146 you mentioned work on making
safesyscalls faster (https://phabricator.haskell.org/D1466 - RFC: foreign import ccall nonblocking).Is this something that should still be done along with changes in here and GHC to address the performance reservations you had back then, or do you think we should switch
unix(or more) tosafeeven if that work remains for later?I suspect for the majority of calls in
unixthe extra overhead ofsafewon't matter. At least from my perspective it would be much better to avoid the unpredictable GC stalls than to save a bit of time on every call.I think it would be nice to work on that patch some more and see if it's worthwhile, but it's probably not something I'll get to unless the performance of safe calls ends up on my critical path somehow.
Carter said:
What’s stopping us from moving the known problematic calls to safe? Their overhead will be on the order of a quarter microsecond. But that’s fine
If it is 200ns or so that would clearly be fine, I'm not sure I completely believe that figure though. A safe call can trigger a context switch, but multiple safe calls will still only trigger a single context switch so it doesn't give you a genuine idea of the overhead to time 10^6 safe calls and then divide the time by 10^6.
The mistake I made back when I did try to fix this was to try to send a single big patch. If flag at build time is (still) OK, then I can send small patches, converting sets of functions over.
Another problem is that I have no idea how this could be tested for correctness at all.
@simonmar - regarding context switches - since many of these calls end up as syscalls, they will involve a context switch anyway, no?
I wonder, for example, if it's ever worth for an
open()call to be marked unsafe, since the slowdown will be userspace/kernel switch, and not just haskell thread switch?@simonmar - regarding context switches - since many of these calls end up as syscalls, they will involve a context switch anyway, no?
I believe system calls are more efficient than context switches.
I wonder, for example, if it's ever worth for an
open()call to be marked unsafe, since the slowdown will be userspace/kernel switch, and not just haskell thread switch?The answer here is to measure it :)
I haven't measured it myself yet, but this answer https://stackoverflow.com/questions/32748530/on-linux-is-access-faster-than-stat/32749760#32749760 suggests that cached syscalls of the
statfamily take 1-4 microseconds. But I don't know on which machine or Linux version, so we should still verify it.If the overhead turns out negligible as we hope for, my favourite outcome would be to use
safeunconditionally, without a flag.I don't think that's a good idea. First, there is the
safevsinterruptibledifference.Even with that aside, for things which don't cause a syscall but instead are pure userspace, using
unsafeseems wasteful, no?That's assuming we can determine what a given libc function does in a reliable way.
If not, I agree, safe is the right answer. Hmm, more convinced now :)
Put another way, I expect proudly probably 60% interruptible calls, 35% safe calls, 5% unsafe (e.g getpid* family).
First, there is the
safevsinterruptibledifference.By all means
interruptibleis even better where supported, but that seems like a follow-up improvement to me.If we find it's easy to do it in the same go, I'm for it (I just found in the past that checking whether
interruptiblewill work is more time intensive than switching fromunsafetosafe, so it may make sense to first do the latter to get rid fo the hangs that arise indpendent of interruptibility).Even with that aside, for things which don't cause a syscall but instead are pure userspace, using
unsafeseems wasteful, no?Sure, we're still talking about your scope from the issue description:
various I/O (in the filesystem sense, not Haskell
IO)Ah I also just noticed I can make a plug here:
Regarding
interruptible, hatrace could be used to test that the interruption signal is indeed delivered to the syscall.If we don't want to add tests in
unix, we could also add them to hatrace's test suite like I did for a GHC bug.- added a commit that references this issue
on Mar 5, 2021 - added a commit that references this issue
on May 15, 2021
Hi,
I'm a bit surprised that various I/O (in the filesystem sense, not Haskell
IO) functions are imported asunsafe. While they won't call back to Haskell, depeding on which filesystem they talk to, they can take arbitrary amounts of time, blocking an execution context for this entire duration.Would it make sense to also have "safe" version of the functions, for the case when performance is less of an issue compared to safety? Would it make sense to at least document that they are imported unsafe?
For the record, I've run a simple benchmark comparing "chmod" performance on various local and remote filesystems and the difference between unsafe and safe is between 10% slower (nfs) to 25% slower (local real filesystems) to 30% (tmpfs). So there is a performance hit indeed.