Opened 3 years ago

Closed 3 years ago

#13246 closed bug (fixed)

hPutBuf issues unnecessary empty write syscalls for large writes

Reported by: nh2 Owned by:
Priority: normal Milestone: 8.2.1
Component: Runtime System Version: 8.0.2
Keywords: Cc: nh2, simonmar
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case:
Blocked By: Blocking:
Related Tickets: #13245 Differential Rev(s): Phab:D3117
Wiki Page:

Description

To get good performance, it is better to use few system calls that write lots of data in batch.

I found a bug in hPutBuf that makes this concept not work: When using hPutBuf with a buffer size greater than 8095 (this number it self is a bug, #13245) bytes, two syscalls are issued instead of one: one empty write("") (a zero-bytes-write, which can't do anything useful), and after that the actual useful write() of the data.

Example code:

main = do
  withBinaryFile "testfile2" WriteMode $ \ hTo -> do
    let bufferSize = 8096
    allocaBytes bufferSize $ \buffer -> do
      Main.hPutBuf hTo buffer bufferSize

In strace -f -T on the compiled binary, we see the syscalls:

write(3, "", 0)                         = 0 <0.000004>
write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8096) = 8096 <0.000017>

As you can see in the timings, this also has a fairly large performance overhead (20% in this case).

When using bufferSize = 8095, the write("") disappears.

The problem is this code for bufWrite (called by hPutBuf):

bufWrite :: Handle__-> Ptr Word8 -> Int -> Bool -> IO Int
bufWrite h_@Handle__{..} ptr count can_block =
  seq count $ do  -- strictness hack
  old_buf@Buffer{ bufRaw=old_raw, bufR=w, bufSize=size }
     <- readIORef haByteBuffer

  -- enough room in handle buffer?
  hPutStrLn System.IO.stderr (show (size, w, count))
  if (size - w > count)
        -- There's enough room in the buffer:
        -- just copy the data in and update bufR.
        then do debugIO ("hPutBuf: copying to buffer, w=" ++ show w)
                copyToRawBuffer old_raw w ptr count
                writeIORef haByteBuffer old_buf{ bufR = w + count }
                return count

        -- else, we have to flush
        else do debugIO "hPutBuf: flushing first"
                old_buf' <- Buffered.flushWriteBuffer haDevice old_buf
                        -- TODO: we should do a non-blocking flush here
                writeIORef haByteBuffer old_buf'
                -- if we can fit in the buffer, then just loop
                if count < size
                   then bufWrite h_ ptr count can_block
                   else if can_block
                           then do writeChunk h_ (castPtr ptr) count
                                   return count
                           else writeChunkNonBlocking h_ (castPtr ptr) count

The check if (size - w > count) should be if (size - w >= count) instead, because we can do the write all fine if it fits exactly.

In the adversarial case, size - w == count, we go into the hPutBuf: flushing first branch, thus emitting the write("").

See https://github.com/ghc/ghc/blame/876b00ba25a615423f48b0cf9d443a9fd5dbd6f4/libraries/base/GHC/IO/Handle/Text.hs#L740 for the full code.

Simon Marlow has confirmed this on IRC, I'll submit a patch for it that switches to >=.

It would be nice if the fix could be released in both GHC 8.2 and and 8.0.3.

Change History (9)

comment:1 Changed 3 years ago by nh2

I've extracted the relevant code into a simple-to-run-and-modify single file, so that we can play around with it without recompiling base:

comment:2 Changed 3 years ago by nh2

When writing the fix, I noticed that my description of what exactly is the bug is wrong.

The if (size - w > count) isn't the problem, that's just another infelicity (e.g. it'll make that if you have an 8 K buffer and write 2 K chunks to it, we'll always flush 6 K buffers), which I'll fix too.

The real bug is simply that we flushWriteBuffer even if w == 0 (handle buffer is empty).

comment:3 Changed 3 years ago by nh2

Differential Rev(s): https://phabricator.haskell.org/D3117

comment:4 Changed 3 years ago by nh2

Status: newpatch

comment:5 Changed 3 years ago by mpickering

Differential Rev(s): https://phabricator.haskell.org/D3117Phab:D3117

comment:6 Changed 3 years ago by Ben Gamari <ben@…>

In 805db965/ghc:

Fix: hPutBuf issues unnecessary empty write syscalls for large writes (#13246)

Until now, any `hPutBuf` that wrote `>= the handle buffer size` would
trigger an unnecessary `write("")` system call before the actual write
system call.

This is fixed by making sure that we never flush an empty handle buffer:
Only flush `when (w > 0)`.

Reviewers: simonmar, austin, hvr, bgamari

Reviewed By: bgamari

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D3119

comment:7 Changed 3 years ago by bgamari

Milestone: 8.2.18.0.3
Status: patchmerge

At the moment we don't have a compelling reason to release a 8.0.3 but I'll milestone it there regardless.

comment:8 Changed 3 years ago by bgamari

Milestone: 8.0.38.2.1

At this point it is rather unlikely that there will be an 8.0.3. Re-milestoning.

comment:9 Changed 3 years ago by bgamari

Resolution: fixed
Status: mergeclosed
Note: See TracTickets for help on using tickets.