safe vs. unsafe foreign imports

[iustin]

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.

[cartazio]

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]

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 unsafeFoo or put in a separate .Unsafe module, with clear documentation about the possible implications.

[gregorycollins]

Agreed, if the call would ever block (and that includes most filesystem functions) that means you want "safe".

[iustin]

Glad to see people agree :)

I'll try and prepare a patch then.

[redneb]

Even worse, open(2) (which is used by openFd) is marked unsafe despite the fact the it can block forever, eg when opening a named pipe with O_RDONLY and 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 in System.Posix.Internals from base.

[hvr]

@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]

@hvr - yep, now that the weekend has arrived I will - I don't quite have time during the week, sorry :)

[iustin]

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.

1. 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.
2. 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
3. Provide a cabal flag to switch between the safe and unsafe alternatives; while this could work for unix, I don't see it working for base, 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 getpid call, which is the most trivial after noop, is 26ns vs 250ns (<10×)
• a getenv call, less trivial but still cheap, is 153ns vs 365ns (2.4×)
• a getgrgid call, 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…

[cartazio]

I'd vote for providing .UnSafe modules 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]

One other option:

• Default to safe for all the dangerous imports
• Provide a compile-time cabal flag to switch to unsafe
• Upload unix-unsafe to Hackage which is the same as unix but with the imports defaulting to unsafe.

[hvr]

@cartazio I'm against .UnSafe modules, 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. ByteString for the payload) operations, that may bring yet another .ByteString... suffix to the hierachy)

@simonmar the main problem I see with unix-unsafe package is that you'd have to depend on unix, as otherwise you'd end up with divergent types, if you need to pass values of types defined in unix to operations of unix-unsafe; this may be quite a maintainance overhead of keeping unix-unsafe in sync w/ unix

Fwiw, I like the idea of a manual cabal flag to switch to the old unsafe semantics, but defaulting everything that's dangerously blocking to safe by 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 an unix-unsafe package if there's really demand for it.

[hvr]

Fyi, as this isn't a new regression we'll probably not target GHC 7.10.1 for this fix

[cartazio]

will making all the current calls use safe be part of 7.10?

[iustin]

@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!