[MLton] cvs commit: MAIL: PosixError.SysCall changes

[Matthew Fluet]

> Posix.Process.pause
> 	Why is there a FIXME comment?

I wanted to raise a question as to the correct behavior of
Posix.Process.pause.  Before this checkin, the code looked like:
  fun pause () = Error.checkResult (Prim.pause ())
Note that this would _always_ raise SysError, because pause is supposed to
return -1 with errno set to EINTR.  Under the new code, we raise an error
if errno is anything but EINTR.  This seems to be better behavior to me.

> Posix.FileSys.{fstat,lstat,stat}
> 	Shouldn't these include the construction of the ST.stat
> 	within the critical section (i.e. use syscall instead of
> 	simple)?

Agreed.

> OS.IO.poll
> 	It seems like the array read after the call should be in the
> 	critical section.

I don't think it needs to be in the critical section.  The reventss array
is local to the poll function, so there is no contention; that is, even if
we switch threads and another thread calls poll, the second thread will
have it's own reventss array.  And, in my mind, the less code in the
critical section, the better.

I've tried to follow that philosophy, by lifting most argument processing
outside the SysCall.* functions.  Mostly, these were NullString.nullTerm
functions.  I left in things that were essentially identity functions, and
sometimes did more processing than was absolutely necessary in post.  I
certainly wasn't religious about it, so there are a few places where
things could be improved a little more.

> 	Also, I hate to throw another wrinkle in things, but it looks
> 	to me like the restarted system call in this case should be
> 	slightly different.  The timeOut should go down by the amount
> 	of time already waited until the interrupt arrived.  I'm not
> 	sure if this is practical, though.

I understand the issue.  And I agree that this may be a situation where we
don't want to restart the system call with exactly the same arguments as
before.  But, we would need to interleave Time.now () calls, and it may
not be practical.

> NetHostDB.getHostName.
> 	It seems like the array read after the call should be in the
> 	critical section.

Again, the arrayis local to the function.  If we rewrote it to:

local
   val n = 128
   val buf = CharArray.array (n, #"\000")
in
   fun getHostName () =
     Posix.Error.SysCall.syscall
     (fn () =>
      (Prim.getHostName (CharArray.toPoly buf, n), fn () =>
       case CharVector.findi (fn (_, c) => c = #"\000") buf of
         NONE => CharArray.vector buf
         SOME n =>
           CharArraySlice.vector (CharArraySlice.slice (buf, 0, SOME n))))
end

Then we would need to use SysCall.syscall

> One general worry I have is the use of {restart = false} to optimize
> system calls which are believed to never return EINTR.  I'm not
> convinced that the speed/code-size gain is worth the potential
> problems caused by missing a case due to incorrect man pages or
> different behavior on another platform.  It seems more stable to err
> conservatively and use the restarting version everywhere.  It would
> also cut the number of special case functions in half, but that's
> minor compared to the correctness argument.

I haven't done any benchmarks or anything, so I don't know if there is an
appreciable slowdown in the new code.  I strongly doubt it, as in the
common case, we are only doing one additional comparison.  And I got a
major speed up in CML by only wrapping print with atomicBegin/atomicEnd
rather than block/unblock.