Subject: bug#32228: sed -i does not buffer, causing excessive writes

From: Assaf Gordon <[email-masked]>
Date: Fri, 20 Jul 2018 19:48:28 -0600

Hello,

On 20/07/18 02:23 PM, Vidar Holen wrote:
> I'm using noticing that `sed -i` does not do any kind of output
> buffering. While behavior is correct, this causes an excessive number of
> small writes:

> I'm seeing this on Debian with the latest sed commit (c52a676) and libc
> 2.27-2. The root cause appears to be `ck_mkstemp` using `fdopen`, which
> unlike `fopen` does not buffer by default.

Thank you for reporting this issue, and providing clear way to reproduce
it. I can confirm seeing the same behavior on Debian 9.4 with glibc 2.24 
(and latest sed).

I think the technical culprit is slightly different:
It is not due to 'ck_mkstemp/fdopen', but because sed explicitly flushes
the output after every line, except if the output is STDOUT.

This happens in execute.c:flush_output():
https://opengrok.housegordon.com/source/xref/sed/sed/execute.c#flush_output

Changing this function enables default buffering with "sed -i"
(using whatever the buffering default is for glibc).

This change does have a side-effect:
It also changes the buffering of other write commands such as 'w', 'W'
and 's///w'.
It might have other side effects I'm haven't spotted.
Based on the ChangeLog-2014 file, I see this function was added in 2003.

I wonder if there are good reasons to explicitly flush all output -
ideas anyone?

If you can give this a quick test and let me know if you also notice
improvements on your system - would be appreciated.

Comments and suggestions welcomed,
  - assaf

>From 993d7fc501788abeef762626d297516698bb09f8 Mon Sep 17 00:00:00 2001
From: Assaf Gordon <[email-masked]>
Date: Fri, 20 Jul 2018 19:24:12 -0600
Subject: [PATCH] sed: do not flush output stream unless in unbuffered mode

Previously sed would explicitly flush the output after
every output line, except if the output was stdout in unbuffered mode.

In practice this was equivalent to forcing line-buffering, and was
espcially was noticable with "sed -i" (where the output is a temporary
file).

With this change, explicit flushing only happens with "sed -u",
regardless of the type of output file, making "sed -i" much faster.
This change also affect other write commands such as 'w'/'W' and 's///w'.

Reported by Vidar Holen <[email-masked]> in
https://lists.gnu.org/r/bug-sed/2018-07/msg00014.html .

* NEWS: Mention this.
* sed/execute.c (flush_output): Never flush output unless in unbuffered
mode, regardless of which file it is.
---
 NEWS          | 7 +++++++
 sed/execute.c | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index ac166dd..de4b8ee 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,13 @@ GNU sed NEWS                                    -*- outline -*-
 
 * Noteworthy changes in release ?.? (????-??-??) [?]
 
+** Improvements
+
+  sed now uses fully-buffered output (instead of line-buffered) when
+  writing to files. This should noticeably improve performance of "sed -i"
+  and other write commands.
+  Buffering can be disabled (as before) with "sed -u".
+
 ** Bug fixes
 
   sed no longer accesses invalid memory (heap overflow) when given invalid
diff --git a/sed/execute.c b/sed/execute.c
index 7a4850f..1cc1d3f 100644
--- a/sed/execute.c
+++ b/sed/execute.c
@@ -415,7 +415,7 @@ output_missing_newline(struct output *outf)
 static inline void
 flush_output(FILE *fp)
 {
-  if (fp != stdout || unbuffered)
+  if (unbuffered)
     ck_fflush(fp);
 }
 
-- 
2.11.0