[libc-commits] [PATCH] D126829: [libc] add buffering to FILE writes

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/File/file.cpp:47
+  // be buffered if there's space.
+  size_t flush_point = bufspace;
+
----------------
While the concept of `flush_point` seems promising, I think there is a mix up of a `xfer_point` and a `flush_point` in the code below. You want to transfer and clear the libc buffer when it is full. But, under `_IOLBF`, you want to actually perform the flush operation. May be I am missing it, but I don't see any call to `platform_flush` after this line.


================
Comment at: libc/src/__support/File/file.cpp:125
+    if (len < flush_point)
+      return len;
+  } else
----------------
It took me some time to understand why this return is OK. Which probably means we need to refactor/rename/comment.


================
Comment at: libc/src/__support/File/file.cpp:126
+      return len;
+  } else
+    write_size = 0; // else no characters were copied.
----------------
If the `if` has braces, `else` should also have braces.


================
Comment at: libc/src/__support/File/file.cpp:147
 
-  size_t transferred =
-      platform_write(this, dataref.data() + write_size, remaining);
-  if (transferred < remaining) {
-    err = true;
-    return write_size + transferred;
+  size_t remaining = len - flush_point;
+
----------------
Should this be:

```
size_t remaining = len - write_size;
```


================
Comment at: libc/src/__support/File/file.cpp:151
+  // write some characters from data directly.
+  if (flush_point > bufspace) {
+    // if the remaining characters after the flush point won't fit in the
----------------
Should `remaining` be updated inside this `if` block? Couple of points:

1. It seems like the condition is true only for a specific case of `_IOLBF`.
1. This condition can be true even if there is no newline char in the input?


================
Comment at: libc/src/__support/File/file.cpp:158
+        platform_write(this, dataref.data() + write_size, rem_write_size);
+    if (transferred < remaining) {
+      err = true;
----------------
This condition can be true if `flush_point - write_size` is less than `remaining`?


================
Comment at: libc/src/__support/File/file.cpp:171
+    pos = remaining; // to reach this point, the buffer must have been flushed.
+  }
+
----------------
Is an `else` missing here?


================
Comment at: libc/src/__support/File/file.cpp:34
+    return written;
+  }
   cpp::ArrayRef<uint8_t> dataref(data, len);
----------------
michaelrj wrote:
> sivachandra wrote:
> > These additional conditionals are making the code even more complicated. Instead, we should do this: rename the current `write_unlocked` method to `write_unlocked_fully_buffered` and make it private. Next, add two new methods like this:
> > 
> > ```
> > public:
> > size_t write_unlocked(const void *data, size_t len) {
> >   if (bufmode == _IOLBF) {
> >     return write_unlocked_line_buffered(data, len);
> >   } else if (bufmode == _IOFBF) {
> >     return write_unlocked_fully_buffered(data, len);
> >   } else {
> >     size_t written_size =platform_write(this, data, len);
> >     // Error handling if platform_write fails.
> > 
> >     platform_flush(this);
> > 
> >     // Error handling if platform_flush fails
> > 
> >     return len;
> >   }
> > }
> > 
> > private:
> > size_t write_unlocked_line_buffered(const void *data, size_t len) {
> >   // Instead of looking for the first new line char, look for the last new line char.
> >   char *last = last_newline(data, len);
> >   if (last == nullptr) {
> >    // If no new line char is found, just do normal buffering.
> >     return write_unlocked_fully_buffered(data, len);
> >   }
> > 
> >   size_t written_size;
> >   if (pos == 0) {
> >     // If there is no content in the buffer, write out the new data.
> >     written_size = platform_write(data, last - data /* size including the new line char */);
> >     // Perform error handling
> >   } else {
> >     // First write out the existing content, and then write out the new data.
> >     platform_write(buf, pos);
> >     // Perform error handling
> >     pos = 0;
> > 
> >     written_size = platform_write(data, last - data);
> >     // Perform error handling.
> >   }
> > 
> > 
> >   platform_flush();
> > 
> >   // Perform error handling
> > 
> >   if (written_size == len)
> >     return len;
> > 
> >   // Remaining data should be buffered.
> >   written_size = write_unlocked_fully_buffered(data + written_size, len - written_size);
> > 
> >   // Perform error handling
> > 
> >   return len;
> > }
> > ```
> I don't think splitting line buffered and fully buffered writes is the correct design. The reason being that their logic is almost identical, only varying in the spot where they flush the buffer. With this idea in mind, I've rewritten the original function to support both types of buffering in a manner closer to your original design. I've also kept unbuffered writes in this function, since it seems odd to split just that  function off on its own if all the other write modes are together.
The concept of a `flush_point` is promising but in the current state of this patch, I am not yet convinced.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126829/new/

https://reviews.llvm.org/D126829