Patchwork [libfortran] Reduce syscalls made

login
register
mail settings
Submitter Janne Blomqvist
Date Nov. 8, 2010, 7:56 p.m.
Message ID <AANLkTi=bNX5O-eDtxBZ+Awqh32oyD+Sna2p4TEw4gpPR@mail.gmail.com>
Download mbox | patch
Permalink /patch/70452/
State New
Headers show

Comments

Janne Blomqvist - Nov. 8, 2010, 7:56 p.m.
Hi,

the attached simple patch slightly reduces the amount of syscalls
gfortran does.  The main issue addressed is that the Fortran standard
forbids opening the same file on multiple units. Hence when opening an
existing file, we need to go through every open file and compare the
device and inode numbers. The patch caches these values when a file is
opened, so it's no longer necessary to call fstat().

Secondly, it's not necessary to call lseek() to test whether a file is
seekable, as that information can already be found in the stat buffer.

Third, when deciding whether to use buffering or not, only buffer
regular files and block devices. While this won't solve the particular
FIFO deadlock issue that was recently reported on the mailing list, it
should at least reduce the chance of similar issues when using special
files. Also, the isatty() test is moved last, doing the cheaper tests
first.

Finally, there is a token effort to support block devices, in case
anyone is crazy enough to attempt accessing such things with gfortran.
This is not tested, and I suspect that stuff will go wrong when trying
to write past EOF.

Tested on x86_64-unknown-linux-gnu, Ok for trunk?

2010-11-08  Janne Blomqvist  <jb@gcc.gnu.org>

	* io/unix.c (struct unix_stream): Add st_dev and st_ino members.
	(fd_to_stream): Avoid unnecessary lseek() call, test isatty()
	last. Make a token effort to support block devices.
	(compare_file_filename): Use cached stat values.
	(find_file0): Likewise.
	(find_file): Likewise.
Jerry DeLisle - Nov. 9, 2010, 1:02 p.m.
On 11/08/2010 11:56 AM, Janne Blomqvist wrote:
> Hi,
>
> the attached simple patch slightly reduces the amount of syscalls
> gfortran does.  The main issue addressed is that the Fortran standard
> forbids opening the same file on multiple units. Hence when opening an
> existing file, we need to go through every open file and compare the
> device and inode numbers. The patch caches these values when a file is
> opened, so it's no longer necessary to call fstat().
>
> Secondly, it's not necessary to call lseek() to test whether a file is
> seekable, as that information can already be found in the stat buffer.
>
> Third, when deciding whether to use buffering or not, only buffer
> regular files and block devices. While this won't solve the particular
> FIFO deadlock issue that was recently reported on the mailing list, it
> should at least reduce the chance of similar issues when using special
> files. Also, the isatty() test is moved last, doing the cheaper tests
> first.
>
> Finally, there is a token effort to support block devices, in case
> anyone is crazy enough to attempt accessing such things with gfortran.
> This is not tested, and I suspect that stuff will go wrong when trying
> to write past EOF.
>
> Tested on x86_64-unknown-linux-gnu, Ok for trunk?
>

OK, thanks!

Jerry
Janne Blomqvist - Nov. 9, 2010, 6:18 p.m.
On Tue, Nov 9, 2010 at 15:02, Jerry DeLisle <jvdelisle@frontier.com> wrote:
> On 11/08/2010 11:56 AM, Janne Blomqvist wrote:
>>
>> Hi,
>>
>> the attached simple patch slightly reduces the amount of syscalls
>> gfortran does.  The main issue addressed is that the Fortran standard
>> forbids opening the same file on multiple units. Hence when opening an
>> existing file, we need to go through every open file and compare the
>> device and inode numbers. The patch caches these values when a file is
>> opened, so it's no longer necessary to call fstat().
>>
>> Secondly, it's not necessary to call lseek() to test whether a file is
>> seekable, as that information can already be found in the stat buffer.
>>
>> Third, when deciding whether to use buffering or not, only buffer
>> regular files and block devices. While this won't solve the particular
>> FIFO deadlock issue that was recently reported on the mailing list, it
>> should at least reduce the chance of similar issues when using special
>> files. Also, the isatty() test is moved last, doing the cheaper tests
>> first.
>>
>> Finally, there is a token effort to support block devices, in case
>> anyone is crazy enough to attempt accessing such things with gfortran.
>> This is not tested, and I suspect that stuff will go wrong when trying
>> to write past EOF.
>>
>> Tested on x86_64-unknown-linux-gnu, Ok for trunk?
>>
>
> OK, thanks!
>
> Jerry
>

Thanks!

Sending        ChangeLog
Sending        io/unix.c
Transmitting file data ..
Committed revision 166502.
Toon Moene - Nov. 15, 2010, 6:54 p.m.
Janne Blomqvist wrote:

> Finally, there is a token effort to support block devices, in case
> anyone is crazy enough to attempt accessing such things with gfortran.
> This is not tested, and I suspect that stuff will go wrong when trying
> to write past EOF.

Just for the record - Control Data's NOS/BE's "fsck" program (aptly 
named "RECOVER") was written in (their) Fortran IV.

The fact that I actually worked on supporting this system (25 years ago) 
must be the reason I got contacted by a recruiter of [company name 
elided] who thought that I would be a nice addition to their embedded 
Linux group (snicker :-)

Patch

diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index e55af18..a384f7b 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -183,7 +183,11 @@  typedef struct
 
   int ndirty;			/* Dirty bytes starting at buffer_offset */
 
-  int special_file;		/* =1 if the fd refers to a special file */
+  int special_file;             /* =1 if the fd refers to a special file */
+
+  /* Cached stat(2) values.  */
+  dev_t st_dev;
+  ino_t st_ino;
 }
 unix_stream;
 
@@ -940,18 +944,29 @@  fd_to_stream (int fd)
 
   fstat (fd, &statbuf);
 
-  if (lseek (fd, 0, SEEK_CUR) == (gfc_offset) -1)
-    s->file_length = -1;
-  else
-    s->file_length = S_ISREG (statbuf.st_mode) ? statbuf.st_size : -1;
-
+  s->st_dev = statbuf.st_dev;
+  s->st_ino = statbuf.st_ino;
   s->special_file = !S_ISREG (statbuf.st_mode);
 
-  if (isatty (s->fd) || options.all_unbuffered
+  if (S_ISREG (statbuf.st_mode))
+    s->file_length = statbuf.st_size;
+  else if (S_ISBLK (statbuf.st_mode))
+    {
+      /* Hopefully more portable than ioctl(fd, BLKGETSIZE64, &size)?  */
+      gfc_offset cur = lseek (fd, 0, SEEK_CUR);
+      s->file_length = lseek (fd, 0, SEEK_END);
+      lseek (fd, cur, SEEK_SET);
+    }
+  else
+    s->file_length = -1;
+
+  if (!(S_ISREG (statbuf.st_mode) || S_ISBLK (statbuf.st_mode))
+      || options.all_unbuffered
       ||(options.unbuffered_preconnected && 
          (s->fd == STDIN_FILENO 
           || s->fd == STDOUT_FILENO 
-          || s->fd == STDERR_FILENO)))
+          || s->fd == STDERR_FILENO))
+      || isatty (s->fd))
     raw_init (s);
   else
     buf_init (s);
@@ -1370,9 +1385,9 @@  int
 compare_file_filename (gfc_unit *u, const char *name, int len)
 {
   char path[PATH_MAX + 1];
-  gfstat_t st1;
+  gfstat_t st;
 #ifdef HAVE_WORKING_STAT
-  gfstat_t st2;
+  unix_stream *s;
 #else
 # ifdef __MINGW32__
   uint64_t id1, id2;
@@ -1385,12 +1400,12 @@  compare_file_filename (gfc_unit *u, const char *name, int len)
   /* If the filename doesn't exist, then there is no match with the
    * existing file. */
 
-  if (stat (path, &st1) < 0)
+  if (stat (path, &st) < 0)
     return 0;
 
 #ifdef HAVE_WORKING_STAT
-  fstat (((unix_stream *) (u->s))->fd, &st2);
-  return (st1.st_dev == st2.st_dev) && (st1.st_ino == st2.st_ino);
+  s = (unix_stream *) (u->s);
+  return (st.st_dev == s->st_dev) && (st.st_ino == s->st_ino);
 #else
 
 # ifdef __MINGW32__
@@ -1432,10 +1447,12 @@  find_file0 (gfc_unit *u, FIND_FILE0_DECL)
     return NULL;
 
 #ifdef HAVE_WORKING_STAT
-  if (u->s != NULL
-      && fstat (((unix_stream *) u->s)->fd, &st[1]) >= 0 &&
-      st[0].st_dev == st[1].st_dev && st[0].st_ino == st[1].st_ino)
-    return u;
+  if (u->s != NULL)
+    {
+      unix_stream *s = (unix_stream *) (u->s);
+      if (st[0].st_dev == s->st_dev && st[0].st_ino == s->st_ino)
+	return u;
+    }
 #else
 # ifdef __MINGW32__ 
   if (u->s && ((id1 = id_from_fd (((unix_stream *) u->s)->fd)) || id1))
@@ -1468,7 +1485,7 @@  gfc_unit *
 find_file (const char *file, gfc_charlen_type file_len)
 {
   char path[PATH_MAX + 1];
-  gfstat_t st[2];
+  gfstat_t st[1];
   gfc_unit *u;
 #if defined(__MINGW32__) && !HAVE_WORKING_STAT
   uint64_t id = 0ULL;