Remove write buffering for IO

* The existing write buffering from Rubinius is buggy, complicated,
  and missing synchronization. Write buffering does not seem needed
  in Ruby (unlike read buffering). It also caused an extra copy of the bytes.
* Also makes it easier to reason about RDWR IOs since now there cannot
  be both directions using the same buffer.
* Ensure every usage of @ibuffer is guarded by #ensure_open_and_readable
  or handles @ibuffer being nil.
* Fixes #2034

Author: eregon

spec/tags/core/io/flush_tags.txt

@@ -0,0 +1 @@
+fails:IO#flush on a pipe raises Errno::EPIPE if sync=false and the read end is closed

src/main/ruby/truffleruby/core/io.rb

@@ -87,10 +87,6 @@ class EINPROGRESSWaitWritable < Errno::EINPROGRESS
   # equals total, no additional bytes will be filled until the
   # buffer is emptied.
   #
-  # As a write buffer, +empty_to+ removes bytes from +start+ up
-  # to +used+. When +start+ equals +used+, no additional bytes
-  # will be emptied until the buffer is filled.
-  #
   # IO presents a stream of input. Buffer presents buckets of
   # input. IO's task is to chain the buckets so the user sees
   # a stream. IO explicitly requests that the buffer be filled
@@ -108,7 +104,6 @@ def initialize
       @start = 0
       @used = 0
       @total = SIZE
-      @write_synced = true
       @eof = false
     end
 
@@ -144,26 +139,6 @@ def reset!
       @start = 0
       @used = 0
       @eof = false
-      @write_synced = true
-    end
-
-    def write_synced?
-      @write_synced
-    end
-
-    # Returns the number of bytes that could be written to the buffer.
-    # If the number is less then the expected, then we need to +empty_to+
-    # the IO, and +unshift+ again beginning at +start_pos+.
-    def unshift(str, start_pos)
-      @write_synced = false
-      free = unused
-      written = str.bytesize - start_pos
-      if written > free
-        written = free
-      end
-      @storage.fill(@used, str, start_pos, written)
-      @used += written
-      written
     end
 
     def fill(io, max = DEFAULT_READ_SIZE)
@@ -192,7 +167,6 @@ def fill(io, max = DEFAULT_READ_SIZE)
     # Returns the number of bytes in the buffer.
     def fill_from(io, skip = nil, max = DEFAULT_READ_SIZE)
       Truffle::System.synchronized(self) do
-        empty_to io
         discard skip if skip
 
         if empty?
@@ -207,16 +181,6 @@ def fill_from(io, skip = nil, max = DEFAULT_READ_SIZE)
       end
     end
 
-    def empty_to(io)
-      return 0 if @write_synced or empty?
-
-      data = String.from_bytearray(@storage, @start, size, Encoding::ASCII_8BIT)
-      Truffle::POSIX.write_string io, data, true
-      reset!
-
-      size
-    end
-
     # Advances the beginning-of-buffer marker past any number
     # of contiguous characters == +skip+. For example, if +skip+
     # is ?\n and the buffer contents are "\n\n\nAbc...", the
@@ -238,8 +202,6 @@ def find(pattern, discard = nil)
 
     def unseek!(io)
       Truffle::System.synchronized(self) do
-        # Unseek the still buffered amount
-        return unless write_synced?
         unless empty?
           amount = @start - @used
           r = Truffle::POSIX.lseek(io.fileno, amount, IO::SEEK_CUR)
@@ -1026,16 +988,19 @@ def self.setup(io, fd, mode=nil, sync=false)
       if cur_mode and (cur_mode == RDONLY or cur_mode == WRONLY) and mode != cur_mode
         raise Errno::EINVAL, "Invalid new mode for existing descriptor #{fd}"
       end
+    else
+      mode = cur_mode or raise 'No mode given for IO'
     end
 
     # Close old descriptor if there was already one associated
     io.close if Primitive.io_fd(io) != -1
 
     Primitive.io_set_fd(io, fd)
-    io.instance_variable_set :@mode, mode || cur_mode
-    io.sync       = !!sync
+    io.instance_variable_set :@mode, mode
+    io.sync = !!sync
     io.autoclose  = true
-    io.instance_variable_set :@ibuffer, IO::InternalBuffer.new
+    ibuffer = mode != WRONLY ? IO::InternalBuffer.new : nil
+    io.instance_variable_set :@ibuffer, ibuffer
     io.instance_variable_set :@lineno, 0
 
     # Truffle: STDOUT isn't defined by the time this call is made during bootstrap, so we need to guard it.
@@ -1156,7 +1121,7 @@ def binmode?
 
   # Used to find out if there is buffered data available.
   private def buffer_empty?
-    @ibuffer.empty?
+    @ibuffer.nil? or @ibuffer.empty?
   end
 
   def close_on_exec=(value)
@@ -1640,7 +1605,6 @@ def fileno
   #  no newline
   def flush
     ensure_open
-    @ibuffer.empty_to self
     self
   end
 
@@ -1658,7 +1622,7 @@ def fsync
   end
 
   def getbyte
-    ensure_open
+    ensure_open_and_readable
     @ibuffer.getbyte(self)
   end
 
@@ -1670,7 +1634,7 @@ def getbyte
   #  f.getc   #=> 84
   #  f.getc   #=> 104
   def getc
-    ensure_open
+    ensure_open_and_readable
     @ibuffer.getchar(self)
   end
 
@@ -1904,7 +1868,7 @@ def read(length=nil, buffer=nil)
   # buffer like readpartial. In this case, read(2) is not called.
   def read_nonblock(size, buffer = nil, exception: true)
     raise ArgumentError, 'illegal read size' if size < 0
-    ensure_open
+    ensure_open_and_readable
     self.nonblock = true
 
     buffer = StringValue buffer if buffer
@@ -2026,7 +1990,7 @@ def readlines(sep_or_limit=$/, limit=nil)
   # blocks on the situation IO#sysread causes Errno::EAGAIN as if the fd is blocking mode.
   def readpartial(size, buffer=nil)
     raise ArgumentError, 'negative string size' unless size >= 0
-    ensure_open
+    ensure_open_and_readable
 
     if buffer
       buffer = StringValue(buffer)
@@ -2091,6 +2055,7 @@ def reopen(other, mode=undefined)
         # We need to use that mode of other here like MRI, and not fcntl(), because fcntl(fd, F_GETFL)
         # gives O_RDWR for the 3 standard IOs, even though they are not bidirectional.
         @mode = other.instance_variable_get :@mode
+        @ibuffer = (@mode & ACCMODE) != WRONLY ? IO::InternalBuffer.new : nil
 
         if io.respond_to?(:path)
           @path = io.path
@@ -2128,6 +2093,7 @@ def reopen(other, mode=undefined)
       Errno.handle if mode < 0
 
       @mode = mode
+      @ibuffer = (@mode & ACCMODE) != WRONLY ? IO::InternalBuffer.new : nil
     end
 
     self
@@ -2137,7 +2103,7 @@ def reopen(other, mode=undefined)
   # Internal method used to reset the state of the buffer, including the
   # physical position in the stream.
   private def reset_buffering
-    @ibuffer.unseek! self
+    @ibuffer.unseek! self if @ibuffer
   end
 
   ##
@@ -2347,6 +2313,7 @@ def sync=(v)
   #  @todo  Improve reading into provided buffer.
   #
   def sysread(number_of_bytes, buffer=undefined)
+    ensure_open_and_readable
     flush
     raise IOError unless @ibuffer.empty?
 
@@ -2370,11 +2337,7 @@ def sysread(number_of_bytes, buffer=undefined)
   #  f.sysread(10)                  #=> "And so on."
   def sysseek(amount, whence=SEEK_SET)
     ensure_open
-    if @ibuffer.write_synced?
-      raise IOError unless @ibuffer.empty?
-    else
-      warn 'sysseek for buffered IO', uplevel: 1
-    end
+    raise IOError unless buffer_empty?
 
     amount = Integer(amount)
     r = Truffle::POSIX.lseek(Primitive.io_fd(self), amount, whence)
@@ -2457,19 +2420,7 @@ def write(data)
       end
     end
 
-    if @sync
-      Truffle::POSIX.write_string self, data, true
-    else
-      reset_buffering
-      bytes_to_write = data.bytesize
-
-      while bytes_to_write > 0
-        bytes_to_write -= @ibuffer.unshift(data, data.bytesize - bytes_to_write)
-        @ibuffer.empty_to self if @ibuffer.full?
-      end
-    end
-
-    data.bytesize
+    Truffle::POSIX.write_string self, data, true
   end
 
   def write_nonblock(data, exception: true)

test/mri/excludes/TestIO.rb

@@ -82,3 +82,4 @@
 exclude :test_write_with_many_arguments, "needs investigation"
 exclude :test_write_with_multiple_arguments, "needs investigation"
 exclude :test_write_with_multiple_arguments_and_buffer, "needs investigation"
+exclude :test_copy_stream_dst_rbuf, "seems to require write buffering"

test/truffle/compiler/can-we-fold-yet.sh

@@ -6,6 +6,7 @@ jt ruby --experimental-options --engine.IterativePartialEscape test/truffle/comp
 
 if ! cmp test/truffle/compiler/can-we-fold-yet/expected.txt actual.txt
 then
+  set +x
   echo Output not as expected
   echo Expected:
   cat test/truffle/compiler/can-we-fold-yet/expected.txt

test/truffle/compiler/can-we-fold-yet/expected.txt

@@ -1,12 +1,12 @@
-> 14
-> 14 + 2
-> [1, 2, 3][1]
-> [1, -2, 3].map(&:abs)[1]
-> rand
-> exit
 Can TruffleRuby constant fold yet?
+> 14
 Yes! Truffle can constant fold this to 14
+> 14 + 2
 Yes! Truffle can constant fold this to 16
+> [1, 2, 3][1]
 Yes! Truffle can constant fold this to 2
+> [1, -2, 3].map(&:abs)[1]
 Yes! Truffle can constant fold this to 2
+> rand
 No :( Truffle can't constant fold that
+> exit