Repository navigation
safe vs. unsafe foreign imports #34
Description
Hi,
I'm a bit surprised that various I/O (in the filesystem sense, not Haskell IO) functions are imported as unsafe. 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.
Activity
cartazio commentedon Jan 1, 2015
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!
simonmar commentedon Jan 5, 2015
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.gregorycollins commentedon Jan 5, 2015
Agreed, if the call would ever block (and that includes most filesystem functions) that means you want "safe".
iustin commentedon Jan 5, 2015
Glad to see people agree :)
I'll try and prepare a patch then.
redneb commentedon Jan 5, 2015
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.hvr commentedon Jan 9, 2015
@Lustin are you going to provide such a patch soon? I think we should try to get this as
unix-2.7.1.1(if we can consider this a patch-level bump bugfix) into GHC 7.10.1RC2 ...iustin commentedon Jan 10, 2015
@hvr - yep, now that the weekend has arrived I will - I don't quite have time during the week, sorry :)
iustin commentedon Jan 11, 2015
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.
unsafe; this would be cleaner since at least all the code is in the same place, and keeps module count saneunix, 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:
void noop() { }) runs in around 20ns unsafe, and 240ns safe; that means, the worst slowdown is 12×getpidcall, which is the most trivial after noop, is 26ns vs 250ns (<10×)getenvcall, less trivial but still cheap, is 153ns vs 365ns (2.4×)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% slowerThese 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…
cartazio commentedon Jan 12, 2015
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.simonmar commentedon Jan 12, 2015
One other option:
unix-unsafeto Hackage which is the same asunixbut with the imports defaulting to unsafe.hvr commentedon Jan 12, 2015
@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.hvr commentedon Jan 12, 2015
Fyi, as this isn't a new regression we'll probably not target GHC 7.10.1 for this fix
cartazio commentedon Jan 12, 2015
will making all the current calls use safe be part of 7.10?
iustin commentedon Jan 12, 2015
@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!
hvr commentedon Jan 12, 2015
@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
cartazio commentedon Nov 19, 2019
iustin commentedon Nov 19, 2019
@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.
0001-WIP.patch.gz
simonmar commentedon Nov 20, 2019
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:
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.
iustin commentedon Nov 20, 2019
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.
iustin commentedon Nov 20, 2019
@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 commentedon Nov 22, 2019
I believe system calls are more efficient than context switches.
The answer here is to measure it :)
nh2 commentedon Nov 22, 2019
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.iustin commentedon Nov 22, 2019
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 :)
iustin commentedon Nov 22, 2019
Put another way, I expect proudly probably 60% interruptible calls, 35% safe calls, 5% unsafe (e.g getpid* family).
nh2 commentedon Nov 22, 2019
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).Sure, we're still talking about your scope from the issue description:
nh2 commentedon Nov 22, 2019
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.Use `safe` calls for FS IO to not block a capability.
Use `safe` calls for FS IO to not block a capability.