diff mbox

libio: Always use 8192 bytes as the default buffer size [BZ #4099]

Message ID 56D08C7C.9080806@redhat.com
State New
Headers show

Commit Message

Florian Weimer Feb. 26, 2016, 5:33 p.m. UTC
This avoids overly large buffers with network file systems which report
very large block sizes.

Remove DEV_TTY_P and related macros, which are now unused.

Florian

Comments

Roland McGrath March 3, 2016, 11:33 p.m. UTC | #1
I think this should be split up and separate parts of it discussed
separately.  

First, let's talk about the rationale for the wfiledoalloc.c changes.
Removing the unused cruft #include's and wrong comment are clearly fine.
Those don't belong with a functional change.  Just do those first by
themselves.

You haven't described the rationale for using the narrow buffer size as the
byte size of the wide buffer.  The existing rationale is fairly obvious:
each wchar_t might correspond to as few bytes as one, so multiplying the
narrow buffer size by sizeof (wchar_t) ensures that the wide buffer is
always big enough to contain the same file data as is the narrow buffer.
What is bad about that rationale and what is your better rationale for
making the two sizes equal?


Thanks,
Roland
Florian Weimer March 4, 2016, 9:17 a.m. UTC | #2
On 03/04/2016 12:33 AM, Roland McGrath wrote:
> I think this should be split up and separate parts of it discussed
> separately.  
> 
> First, let's talk about the rationale for the wfiledoalloc.c changes.
> Removing the unused cruft #include's and wrong comment are clearly fine.
> Those don't belong with a functional change.  Just do those first by
> themselves.

You are right, I 'll separate this part and commit it.

> You haven't described the rationale for using the narrow buffer size as the
> byte size of the wide buffer.  The existing rationale is fairly obvious:
> each wchar_t might correspond to as few bytes as one, so multiplying the
> narrow buffer size by sizeof (wchar_t) ensures that the wide buffer is
> always big enough to contain the same file data as is the narrow buffer.

Then why don't we do this for a user-supplied buffer?

> What is bad about that rationale and what is your better rationale for
> making the two sizes equal?

I can skip this part.  I wanted to avoid the substantial memory
allocation (32768 bytes).  I doubt that performance increases with a
larger buffer because it approaches the size of typical first-level caches.

We can post-pone the wide changes.  The important change is the fix for
bug 4099.  Do you think this is controversial as well?  libstdc++
already uses a buffer size of 8192 for its streams (or 8191, depending
on how you look at it).

Florian
diff mbox

Patch

2016-02-26  Florian Weimer  <fweimer@redhat.com>

	* libio/filedoalloc.c (isatty): Remove.
	(local_isatty): Add comment.  Call __isatty directly.
	(_IO_file_doallocate): Update comment.  Always use _IO_BUFSIZ as
	the buffer size.
	* libio/wfiledoalloc.c (isatty): Remove.
	(_IO_wfile_doallocate): Update comment.  Always follow the narrow
	buffer size.
	* sysdeps/mach/hurd/device-nrs.h: Remove file.
	* sysdeps/generic/device-nrs.h (DEV_TTY_P): Remove.
	* sysdeps/unix/sysv/linux/device-nrs.h (DEV_TTY_LOW_MAJOR)
	(DEV_TTY_HIGH_MAJOR, DEV_TTY_P): Remove.

diff --git a/NEWS b/NEWS
index a06a42f..5a4ac4e 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,15 @@  Version 2.24
 * The readdir_r and readdir64_r functions have been deprecated.  It is
   recommended to use readdir and readdir64 instead.
 
+* Byte-oriented stdio streams use a default buffer size of 8192 bytes.
+  Previously, on Linux, the default buffer size on most file systems was
+  4096 bytes, except on network file systems, where the buffer size was
+  unpredictable and could be as large as several megabytes.  The default
+  buffer size for wide-oriented streams now follows the byte-oriented buffer
+  size.  Previously, it could be four times as large as that.
+
+The default buffer size
+
 Security related changes:
 
   [Add security related changes here]
diff --git a/libio/filedoalloc.c b/libio/filedoalloc.c
index b51e910..ce704ff 100644
--- a/libio/filedoalloc.c
+++ b/libio/filedoalloc.c
@@ -55,48 +55,25 @@ 
 
 /* Modified for GNU iostream by Per Bothner 1991, 1992. */
 
-#ifndef _POSIX_SOURCE
-# define _POSIX_SOURCE
-#endif
 #include "libioP.h"
-#include <sys/types.h>
-#include <sys/stat.h>
 #include <stdlib.h>
 #include <unistd.h>
 
-#ifdef _LIBC
-# undef isatty
-# define isatty(Fd) __isatty (Fd)
-
-# include <device-nrs.h>
-#endif
-
-
+/* Return the result of isatty, without changing errno.  */
 static int
 local_isatty (int fd)
 {
   int save_errno = errno;
-  int res = isatty (fd);
+  int res = __isatty (fd);
   __set_errno (save_errno);
   return res;
 }
 
-
-/*
- * Allocate a file buffer, or switch to unbuffered I/O.
- * Per the ANSI C standard, ALL tty devices default to line buffered.
- *
- * As a side effect, we set __SOPT or __SNPT (en/dis-able fseek
- * optimisation) right after the _fstat() that finds the buffer size.
- */
-
+/* Allocate a file buffer, or switch to unbuffered I/O.  Streams for
+ * TTY devices default to line buffered.  */
 int
 _IO_file_doallocate (_IO_FILE *fp)
 {
-  _IO_size_t size;
-  char *p;
-  struct stat64 st;
-
 #ifndef _LIBC
   /* If _IO_cleanup_registration_needed is non-zero, we should call the
      function it points to.  This is to make sure _IO_cleanup gets called
@@ -106,28 +83,13 @@  _IO_file_doallocate (_IO_FILE *fp)
     (*_IO_cleanup_registration_needed) ();
 #endif
 
-  size = _IO_BUFSIZ;
-  if (fp->_fileno >= 0 && __builtin_expect (_IO_SYSSTAT (fp, &st), 0) >= 0)
-    {
-      if (S_ISCHR (st.st_mode))
-	{
-	  /* Possibly a tty.  */
-	  if (
-#ifdef DEV_TTY_P
-	      DEV_TTY_P (&st) ||
-#endif
-	      local_isatty (fp->_fileno))
-	    fp->_flags |= _IO_LINE_BUF;
-	}
-#if _IO_HAVE_ST_BLKSIZE
-      if (st.st_blksize > 0)
-	size = st.st_blksize;
-#endif
-    }
-  p = malloc (size);
+  /* Switch to line buffering for TTYs.  */
+  if (fp->_fileno >= 0 && local_isatty (fp->_fileno))
+    fp->_flags |= _IO_LINE_BUF;
+  char *p = malloc (_IO_BUFSIZ);
   if (__glibc_unlikely (p == NULL))
     return EOF;
-  _IO_setb (fp, p, p + size, 1);
+  _IO_setb (fp, p, p + _IO_BUFSIZ, 1);
   return 1;
 }
 libc_hidden_def (_IO_file_doallocate)
diff --git a/libio/wfiledoalloc.c b/libio/wfiledoalloc.c
index 28c10b6..55bab6c 100644
--- a/libio/wfiledoalloc.c
+++ b/libio/wfiledoalloc.c
@@ -55,26 +55,11 @@ 
 
 /* Modified for GNU iostream by Per Bothner 1991, 1992. */
 
-#ifndef _POSIX_SOURCE
-# define _POSIX_SOURCE
-#endif
 #include "libioP.h"
-#include <sys/types.h>
-#include <sys/stat.h>
 #include <stdlib.h>
-#include <unistd.h>
-
-#ifdef _LIBC
-# undef isatty
-# define isatty(Fd) __isatty (Fd)
-#endif
 
 /*
  * Allocate a file buffer, or switch to unbuffered I/O.
- * Per the ANSI C standard, ALL tty devices default to line buffered.
- *
- * As a side effect, we set __SOPT or __SNPT (en/dis-able fseek
- * optimisation) right after the _fstat() that finds the buffer size.
  */
 
 int
@@ -87,13 +72,11 @@  _IO_wfile_doallocate (_IO_FILE *fp)
   if (fp->_IO_buf_base == NULL)
     _IO_file_doallocate (fp);
 
-  /* If narrow buffer is user allocated (set by setvbuf etc.),
-     use that size as the size of the wide buffer, when it is
-     allocated by _IO_file_doallocate, multiply that by size
-     of the wide character.  */
+  /* Use the size of the narrow buffer as the size of the wide buffer
+     (both measured in bytes).  Round up the wide buffer size to the
+     next multiple of sizeof (wchar_t) if necessary.  */
   size = fp->_IO_buf_end - fp->_IO_buf_base;
-  if ((fp->_flags & _IO_USER_BUF))
-    size = (size + sizeof (wchar_t) - 1) / sizeof (wchar_t);
+  size = (size + sizeof (wchar_t) - 1) / sizeof (wchar_t);
   p = malloc (size * sizeof (wchar_t));
   if (__glibc_unlikely (p == NULL))
     return EOF;
diff --git a/sysdeps/generic/device-nrs.h b/sysdeps/generic/device-nrs.h
index 236d821..9d77318 100644
--- a/sysdeps/generic/device-nrs.h
+++ b/sysdeps/generic/device-nrs.h
@@ -21,7 +21,4 @@ 
 
 /* By default we know no device numbers.  */
 
-/* We cannot check whether a given device is a tty.  */
-#define DEV_TTY_P(statp) (0)
-
 #endif	/* device-nrs.h */
diff --git a/sysdeps/mach/hurd/device-nrs.h b/sysdeps/mach/hurd/device-nrs.h
deleted file mode 100644
index 6a56af1..0000000
--- a/sysdeps/mach/hurd/device-nrs.h
+++ /dev/null
@@ -1,27 +0,0 @@ 
-/* Device numbers of devices used in the implementation.  Hurd version.
-   Copyright (C) 2001-2016 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#ifndef _DEVICE_NRS_H
-#define _DEVICE_NRS_H	1
-
-#include <hurd/hurd_types.h>
-
-/* Check whether a given device is a tty.  */
-#define DEV_TTY_P(statp) ((statp)->st_fstype == FSTYPE_TERM)
-
-#endif	/* device-nrs.h */
diff --git a/sysdeps/unix/sysv/linux/device-nrs.h b/sysdeps/unix/sysv/linux/device-nrs.h
index 78fc79c..2f13147 100644
--- a/sysdeps/unix/sysv/linux/device-nrs.h
+++ b/sysdeps/unix/sysv/linux/device-nrs.h
@@ -19,8 +19,6 @@ 
 #ifndef _DEVICE_NRS_H
 #define _DEVICE_NRS_H	1
 
-#include <sys/sysmacros.h>
-
 /* /dev/null is (1,3).  */
 #define DEV_NULL_MAJOR	1
 #define DEV_NULL_MINOR	3
@@ -29,17 +27,4 @@ 
 #define DEV_FULL_MAJOR	1
 #define DEV_FULL_MINOR	7
 
-/* Pseudo tty slaves.  For Linux we use the Unix98 ttys.  We could
-   also include the old BSD-style tty buts they should not be used and
-   the extra test would only slow down correctly set up systems.  If a
-   system still uses those device the slower tests performed (using
-   isatty) will catch it.  */
-#define DEV_TTY_LOW_MAJOR	136
-#define DEV_TTY_HIGH_MAJOR	143
-
-/* Test whether given device is a tty.  */
-#define DEV_TTY_P(statp) \
-  ({ int __dev_major = major ((statp)->st_rdev);			      \
-     __dev_major >= DEV_TTY_LOW_MAJOR && __dev_major <= DEV_TTY_HIGH_MAJOR; })
-
 #endif	/* device-nrs.h */