Patchwork [4.6,libgfortran/io] Fix build issue with _commit [PR5016]

login
register
mail settings
Submitter Tobias Burnus
Date Oct. 19, 2011, 5:11 p.m.
Message ID <4E9F04A6.2080302@net-b.de>
Download mbox | patch
Permalink /patch/120661/
State New
Headers show

Comments

Tobias Burnus - Oct. 19, 2011, 5:11 p.m.
I intent to commit this patch - created mostly by Janne - soon to 
gcc-4_6-branch; it should solve a build issue.

The patch was build and regtested on x86-64-linux and build and tested 
on MinGW and Cygwin by Kai.

It is a follow up to http://gcc.gnu.org/ml/fortran/2011-10/msg00132.html 
/ http://gcc.gnu.org/ml/gcc-cvs/2011-10/msg00734.html

Please speak up immediately, if you have objects against the committal. 
(Another option would be to revert previous patch, which solved a severe 
performance regression on Windows - thus that patch plus this patch is 
better.)

Sorry for disrupting the 4.6.2 process a bit.

Tobias
Steve Kargl - Oct. 19, 2011, 6:32 p.m.
On Wed, Oct 19, 2011 at 07:11:02PM +0200, Tobias Burnus wrote:
> I intent to commit this patch - created mostly by Janne - soon to 
> gcc-4_6-branch; it should solve a build issue.
> 
> The patch was build and regtested on x86-64-linux and build and tested 
> on MinGW and Cygwin by Kai.
> 
> It is a follow up to http://gcc.gnu.org/ml/fortran/2011-10/msg00132.html 
> / http://gcc.gnu.org/ml/gcc-cvs/2011-10/msg00734.html
> 
> Please speak up immediately, if you have objects against the committal. 
> (Another option would be to revert previous patch, which solved a severe 
> performance regression on Windows - thus that patch plus this patch is 
> better.)
> 
> Sorry for disrupting the 4.6.2 process a bit.
> 

I don't have an objection, just a question.  Does
this issue appear on trunk?  Comment #10 in PR 50016
appears to suggest it does.  In which case, I object
to the initial commit into the 4.6-branch without
testing the patch of trunk first.  Rushing a patch
into a branch to beat a release dead line is simply
a poor choice.
Tobias Burnus - Oct. 19, 2011, 8:38 p.m.
Pre-remark: Jakub (4.6.2 RM) had no objects - and thus I have committed 
it to the branch.

Steve Kargl wrote:
> I don't have an objection, just a question. Does this issue appear on 
> trunk?

Yes and no. The MinGW build issue this patch fixes was unique to the branch.

However, the actual issue - the slow down - exists both on the 4.6 
branch and on the trunk. The problem is that Janne and I couldn't agree 
when to call _commit(). We solved that by deferring it to later for the 
trunk (still time before the 4.7.0 release) while we wanted to have some 
fix for 4.6.2 - thus, we agreed on some fix for 4.6. The fix on the 
branch now calls significatly less often _commit() than before - but it 
probably still calls it too often.

Regarding rushing with the fix: I agree that is was less optimal; the 
main problem is that we do not have any MinGW gfortran developer who can 
properly test such patches and who can detect such regressions more 
timely. (The performance regression dates back over a year.)

Tobias

Patch

libgfortran/
2011-10-19  Janne Blomqvist  <jb@gcc.gnu.org>
	    Kai Tietz  <ktietz@redhat.com>
	    Tobias Burnus  <burnus@net-b.de>

	PR fortran/50016
	* io/unix.h (flush_sync): Add new libgfortran-internal prototype.
	* io/unix.c (flush_sync): New function, which calls sflush and
	on MinGW(-w64) also _commit.
	(flush_all_units, flush_all_units_1): Replace sflush/_commit by
	flush_sync.
	* io/file_pos.c (st_flush): Replace sflush/_commit by flush_sync.
	* io/intrinsics.c (flush_i4, flush_i8): Ditto.

Index: libgfortran/io/file_pos.c
===================================================================
--- libgfortran/io/file_pos.c	(revision 180180)
+++ libgfortran/io/file_pos.c	(working copy)
@@ -451,11 +451,7 @@  st_flush (st_parameter_filepos *fpp)
       if (u->flags.form == FORM_FORMATTED)
         fbuf_flush (u, u->mode);
 
-      sflush (u->s);
-#ifdef _WIN32
-      /* Without _commit, changes are not visible to other file descriptors.  */
-      _commit (u->s->fd);
-#endif
+      flush_sync (u->s);
       unlock_unit (u);
     }
   else
Index: libgfortran/io/unix.c
===================================================================
--- libgfortran/io/unix.c	(revision 180180)
+++ libgfortran/io/unix.c	(working copy)
@@ -1522,6 +1522,23 @@  retry:
   return u;
 }
 
+
+/* Flush dirty data, making sure that OS metadata is updated as
+   well. Note that this is VERY slow on mingw due to committing data
+   to stable storage.  */
+int
+flush_sync (stream * s)
+{
+  if (sflush (s) == -1)
+    return -1;
+#ifdef __MINGW32__
+  if (_commit (((unix_stream *)s)->fd) == -1)
+    return -1;
+#endif
+  return 0;
+}
+
+
 static gfc_unit *
 flush_all_units_1 (gfc_unit *u, int min_unit)
 {
@@ -1538,14 +1555,7 @@  flush_all_units_1 (gfc_unit *u, int min_unit)
 	  if (__gthread_mutex_trylock (&u->lock))
 	    return u;
 	  if (u->s)
-	    {
-	      sflush (u->s);
-#ifdef _WIN32
-	      /* Without _commit, changes are not visible to other
-		 file descriptors.  */
-	      _commit (u->s->fd);
-#endif
-	    }
+	    flush_sync (u->s);
 	  __gthread_mutex_unlock (&u->lock);
 	}
       u = u->right;
@@ -1575,12 +1585,7 @@  flush_all_units (void)
 
       if (u->closed == 0)
 	{
-	  sflush (u->s);
-#ifdef _WIN32
-	  /* Without _commit, changes are not visible to other
-	     file descriptors.  */
-	  _commit (u->s->fd);
-#endif
+	  flush_sync (u->s);
 	  __gthread_mutex_lock (&unit_lock);
 	  __gthread_mutex_unlock (&u->lock);
 	  (void) predec_waiting_locked (u);
Index: libgfortran/io/intrinsics.c
===================================================================
--- libgfortran/io/intrinsics.c	(revision 180180)
+++ libgfortran/io/intrinsics.c	(working copy)
@@ -206,12 +206,7 @@  flush_i4 (GFC_INTEGER_4 *unit)
       us = find_unit (*unit);
       if (us != NULL)
 	{
-	  sflush (us->s);
-#ifdef _WIN32
-	  /* Without _commit, changes are not visible
-	     to other file descriptors.  */
-	  _commit (u->s->fd);
-#endif
+	  flush_sync (us->s);
 	  unlock_unit (us);
 	}
     }
@@ -234,12 +229,7 @@  flush_i8 (GFC_INTEGER_8 *unit)
       us = find_unit (*unit);
       if (us != NULL)
 	{
-	  sflush (us->s);
-#ifdef _WIN32
-	  /* Without _commit, changes are not visible
-	     to other file descriptors.  */
-	  _commit (u->s->fd);
-#endif
+	  flush_sync (us->s);
 	  unlock_unit (us);
 	}
     }
Index: libgfortran/io/unix.h
===================================================================
--- libgfortran/io/unix.h	(revision 180180)
+++ libgfortran/io/unix.h	(working copy)
@@ -167,6 +167,9 @@  internal_proto(is_special);
 extern void flush_if_preconnected (stream *);
 internal_proto(flush_if_preconnected);
 
+extern int flush_sync (stream *);
+internal_proto(flush_sync);
+
 extern int stream_isatty (stream *);
 internal_proto(stream_isatty);