public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH, libfortran] PR 48587 Newunit allocator
@ 2016-10-13 15:16 Janne Blomqvist
  2016-10-13 17:16 ` Jerry DeLisle
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Janne Blomqvist @ 2016-10-13 15:16 UTC (permalink / raw)
  To: fortran, gcc-patches; +Cc: Janne Blomqvist

Currently GFortran newer reuses unit numbers allocated with NEWUNIT=,
instead having a simple counter that is decremented each time such a
unit is opened.  For a long running program which repeatedly opens
files with NEWUNIT= and closes them, the counter can wrap around and
cause an abort.  This patch replaces the counter with an allocator
that keeps track of which units numbers are allocated, and can reuse
them once they have been deallocated.  Since operating systems tend to
limit the number of simultaneous open files for a process to a
relatively modest number, a relatively simple approach with a linear
scan through an array suffices.  Though as a small optimization there
is a low water indicator keeping track of the index for which all unit
numbers below are already allocated.  This linear scan also ensures
that we always allocate the smallest available unit number.

2016-10-13  Janne Blomqvist  <jb@gcc.gnu.org>

	PR libfortran/48587
	* io/io.h (get_unique_unit_number): Remove prototype.
	(newunit_alloc): New prototype.
	* io/open.c (st_open): Call newunit_alloc.
	* io/unit.c (newunits,newunit_size,newunit_lwi): New static
	variables.
	(GFC_FIRST_NEWUNIT): Rename to NEWUNIT_START.
	(next_available_newunit): Remove variable.
	(get_unit): Call newunit_alloc.
	(close_unit_1): Call newunit_free.
	(close_units): Free newunits array.
	(get_unique_number): Remove function.
	(newunit_alloc): New function.
	(newunit_free): New function.

Regtested on x86_64-pc-linux-gnu. Ok for trunk?
---
 libgfortran/io/io.h   |   5 ++-
 libgfortran/io/open.c |   2 +-
 libgfortran/io/unit.c | 103 ++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 86 insertions(+), 24 deletions(-)

diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index ea93fba..aaacc08 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -715,8 +715,9 @@ internal_proto (finish_last_advance_record);
 extern int unit_truncate (gfc_unit *, gfc_offset, st_parameter_common *);
 internal_proto (unit_truncate);
 
-extern GFC_INTEGER_4 get_unique_unit_number (st_parameter_common *);
-internal_proto(get_unique_unit_number);
+extern int newunit_alloc (void);
+internal_proto(newunit_alloc);
+
 
 /* open.c */
 
diff --git a/libgfortran/io/open.c b/libgfortran/io/open.c
index d074b02..2e7163d 100644
--- a/libgfortran/io/open.c
+++ b/libgfortran/io/open.c
@@ -812,7 +812,7 @@ st_open (st_parameter_open *opp)
   if ((opp->common.flags & IOPARM_LIBRETURN_MASK) == IOPARM_LIBRETURN_OK)
     {
       if ((opp->common.flags & IOPARM_OPEN_HAS_NEWUNIT))
-	opp->common.unit = get_unique_unit_number(&opp->common);
+	opp->common.unit = newunit_alloc ();
       else if (opp->common.unit < 0)
 	{
 	  u = find_unit (opp->common.unit);
diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c
index 274b24b..cc24ca7 100644
--- a/libgfortran/io/unit.c
+++ b/libgfortran/io/unit.c
@@ -29,6 +29,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #include "unix.h"
 #include <stdlib.h>
 #include <string.h>
+#include <assert.h>
 
 
 /* IO locking rules:
@@ -68,12 +69,34 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
    on it.  unlock_unit or close_unit must be always called only with the
    private lock held.  */
 
-/* Subroutines related to units */
 
-/* Unit number to be assigned when NEWUNIT is used in an OPEN statement.  */
-#define GFC_FIRST_NEWUNIT -10
+
+/* Table of allocated newunit values.  A simple solution would be to
+   map OS file descriptors (fd's) to unit numbers, e.g. with newunit =
+   -fd - 2, however that doesn't work since Fortran allows an existing
+   unit number to be reassociated with a new file. Thus the simple
+   approach may lead to a situation where we'd try to assign a
+   (negative) unit number which already exists. Hence we must keep
+   track of allocated newunit values ourselves. This is the purpose of
+   the newunits array. The indices map to newunit values as newunit =
+   -index + NEWUNIT_FIRST. E.g. newunits[0] having the value true
+   means that a unit with number NEWUNIT_FIRST exists. Similar to
+   POSIX file descriptors, we always allocate the lowest (in absolute
+   value) available unit number.
+ */
+static bool *newunits;
+static int newunit_size; /* Total number of elements in the newunits array.  */
+/* Low water indicator for the newunits array. Below the LWI all the
+   units are allocated, above and equal to the LWI there may be both
+   allocated and free units. */
+static int newunit_lwi;
+static void newunit_free (int);
+
+/* Unit numbers assigned with NEWUNIT start from here.  */
+#define NEWUNIT_START -10
+
+
 #define NEWUNIT_STACK_SIZE 16
-static GFC_INTEGER_4 next_available_newunit = GFC_FIRST_NEWUNIT;
 
 /* A stack to save previously used newunit-assigned unit numbers to
    allow them to be reused without reallocating the gfc_unit structure
@@ -81,6 +104,7 @@ static GFC_INTEGER_4 next_available_newunit = GFC_FIRST_NEWUNIT;
 static gfc_saved_unit newunit_stack[NEWUNIT_STACK_SIZE];
 static int newunit_tos = 0; /* Index to Top of Stack.  */
 
+
 #define CACHE_SIZE 3
 static gfc_unit *unit_cache[CACHE_SIZE];
 gfc_offset max_offset;
@@ -551,7 +575,7 @@ get_unit (st_parameter_dt *dtp, int do_create)
       if ((dtp->common.flags & IOPARM_DT_HAS_UDTIO) != 0)
 	{
 	  dtp->u.p.unit_is_internal = 1;
-	  dtp->common.unit = get_unique_unit_number (&dtp->common);
+	  dtp->common.unit = newunit_alloc ();
 	  unit = get_gfc_unit (dtp->common.unit, do_create);
 	  set_internal_unit (dtp, unit, kind);
 	  fbuf_init (unit, 128);
@@ -567,7 +591,7 @@ get_unit (st_parameter_dt *dtp, int do_create)
 	    }
 	  else
 	    {
-	      dtp->common.unit = get_unique_unit_number (&dtp->common);
+	      dtp->common.unit = newunit_alloc ();
 	      unit = xcalloc (1, sizeof (gfc_unit));
 	      fbuf_init (unit, 128);
 	    }
@@ -734,6 +758,9 @@ close_unit_1 (gfc_unit *u, int locked)
   free_format_hash_table (u);
   fbuf_destroy (u);
 
+  if (u->unit_number <= NEWUNIT_START)
+    newunit_free (u->unit_number);
+
   if (!locked)
     __gthread_mutex_unlock (&u->lock);
 
@@ -788,6 +815,9 @@ close_units (void)
 	free (newunit_stack[newunit_tos].unit->s);
 	free (newunit_stack[newunit_tos--].unit);
       }
+
+  free (newunits);
+
 #ifdef HAVE_FREELOCALE
   freelocale (c_locale);
 #endif
@@ -885,25 +915,56 @@ finish_last_advance_record (gfc_unit *u)
   fbuf_flush (u, u->mode);
 }
 
+
 /* Assign a negative number for NEWUNIT in OPEN statements or for
    internal units.  */
-GFC_INTEGER_4
-get_unique_unit_number (st_parameter_common *common)
+int
+newunit_alloc (void)
 {
-  GFC_INTEGER_4 num;
-
-#ifdef HAVE_SYNC_FETCH_AND_ADD
-  num = __sync_fetch_and_add (&next_available_newunit, -1);
-#else
   __gthread_mutex_lock (&unit_lock);
-  num = next_available_newunit--;
-  __gthread_mutex_unlock (&unit_lock);
-#endif
-  /* Do not allow NEWUNIT numbers to wrap.  */
-  if (num > GFC_FIRST_NEWUNIT)
+  if (!newunits)
     {
-      generate_error (common, LIBERROR_INTERNAL, "NEWUNIT exhausted");
-      return 0;
+      newunits = xcalloc (32, 1);
+      newunit_size = 32;
     }
-  return num;
+
+  /* Search for the next available newunit.  */
+  for (int ii = newunit_lwi; ii < newunit_size; ii++)
+    {
+      if (!newunits[ii])
+        {
+          newunits[ii] = true;
+          newunit_lwi = ii + 1;
+	  __gthread_mutex_unlock (&unit_lock);
+          return -ii + NEWUNIT_START;
+        }
+    }
+
+  /* Search failed, bump size of array and allocate the first
+     available unit.  */
+  int old_size = newunit_size;
+  newunit_size *= 2;
+  void *p = realloc (newunits, newunit_size);
+  if (!p)
+    abort ();
+  newunits = p;
+  memset (newunits + old_size, 0, old_size);
+  newunits[old_size] = true;
+  newunit_lwi = old_size + 1;
+    __gthread_mutex_unlock (&unit_lock);
+  return -old_size + NEWUNIT_START;
+}
+
+
+/* Free a previously allocated newunit= unit number.  unit_lock must
+   be held when calling.  */
+
+static void
+newunit_free (int unit)
+{
+  int ind = -unit + NEWUNIT_START;
+  assert(ind >= 0 && ind < newunit_size);
+  newunits[ind] = false;
+  if (ind < newunit_lwi)
+    newunit_lwi = ind;
 }
-- 
2.7.4

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH, libfortran] PR 48587 Newunit allocator
  2016-10-13 15:16 [PATCH, libfortran] PR 48587 Newunit allocator Janne Blomqvist
@ 2016-10-13 17:16 ` Jerry DeLisle
  2016-10-13 20:08 ` Jerry DeLisle
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Jerry DeLisle @ 2016-10-13 17:16 UTC (permalink / raw)
  To: Janne Blomqvist, fortran, gcc-patches

On 10/13/2016 08:16 AM, Janne Blomqvist wrote:
> Currently GFortran newer reuses unit numbers allocated with NEWUNIT=,
> instead having a simple counter that is decremented each time such a
> unit is opened.

I am going to have to study this a bit. After I added the newunit stack, the 
units are reused,  Need to see how this plays with internal units, and 
recursive, and dtio.

Jerry

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH, libfortran] PR 48587 Newunit allocator
  2016-10-13 15:16 [PATCH, libfortran] PR 48587 Newunit allocator Janne Blomqvist
  2016-10-13 17:16 ` Jerry DeLisle
@ 2016-10-13 20:08 ` Jerry DeLisle
  2016-10-14 17:02   ` Bernhard Reutner-Fischer
  2016-10-14 21:48 ` Janne Blomqvist
  2016-10-18  9:10 ` Steven Bosscher
  3 siblings, 1 reply; 10+ messages in thread
From: Jerry DeLisle @ 2016-10-13 20:08 UTC (permalink / raw)
  To: Janne Blomqvist, fortran, gcc-patches

On 10/13/2016 08:16 AM, Janne Blomqvist wrote:
> Currently GFortran newer reuses unit numbers allocated with NEWUNIT=,
> instead having a simple counter that is decremented each time such a
> unit is opened.  For a long running program which repeatedly opens
> files with NEWUNIT= and closes them, the counter can wrap around and
> cause an abort.  This patch replaces the counter with an allocator
> that keeps track of which units numbers are allocated, and can reuse
> them once they have been deallocated.  Since operating systems tend to
> limit the number of simultaneous open files for a process to a
> relatively modest number, a relatively simple approach with a linear
> scan through an array suffices.  Though as a small optimization there
> is a low water indicator keeping track of the index for which all unit
> numbers below are already allocated.  This linear scan also ensures
> that we always allocate the smallest available unit number.
>
> 2016-10-13  Janne Blomqvist  <jb@gcc.gnu.org>
>
> 	PR libfortran/48587
> 	* io/io.h (get_unique_unit_number): Remove prototype.
> 	(newunit_alloc): New prototype.
> 	* io/open.c (st_open): Call newunit_alloc.
> 	* io/unit.c (newunits,newunit_size,newunit_lwi): New static
> 	variables.
> 	(GFC_FIRST_NEWUNIT): Rename to NEWUNIT_START.
> 	(next_available_newunit): Remove variable.
> 	(get_unit): Call newunit_alloc.
> 	(close_unit_1): Call newunit_free.
> 	(close_units): Free newunits array.
> 	(get_unique_number): Remove function.
> 	(newunit_alloc): New function.
> 	(newunit_free): New function.
>
> Regtested on x86_64-pc-linux-gnu. Ok for trunk?
>

Yes, OK, clever! Thanks!

Jerry

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH, libfortran] PR 48587 Newunit allocator
  2016-10-13 20:08 ` Jerry DeLisle
@ 2016-10-14 17:02   ` Bernhard Reutner-Fischer
  2016-10-14 20:41     ` Janne Blomqvist
  0 siblings, 1 reply; 10+ messages in thread
From: Bernhard Reutner-Fischer @ 2016-10-14 17:02 UTC (permalink / raw)
  To: Jerry DeLisle, Janne Blomqvist, fortran, gcc-patches

On 13 October 2016 22:08:21 CEST, Jerry DeLisle <jvdelisle@charter.net> wrote:
>On 10/13/2016 08:16 AM, Janne Blomqvist wrote:

>>
>> Regtested on x86_64-pc-linux-gnu. Ok for trunk?
>>
>
>Yes, OK, clever! Thanks!

Is 32 something a typical program uses?
I'd have started at 8 and had not doubled but += 16 fwiw.

Cheers

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH, libfortran] PR 48587 Newunit allocator
  2016-10-14 17:02   ` Bernhard Reutner-Fischer
@ 2016-10-14 20:41     ` Janne Blomqvist
  2016-10-15  1:15       ` Bernhard Reutner-Fischer
  0 siblings, 1 reply; 10+ messages in thread
From: Janne Blomqvist @ 2016-10-14 20:41 UTC (permalink / raw)
  To: Bernhard Reutner-Fischer; +Cc: Jerry DeLisle, Fortran List, GCC Patches

On Fri, Oct 14, 2016 at 8:01 PM, Bernhard Reutner-Fischer
<rep.dot.nop@gmail.com> wrote:
> On 13 October 2016 22:08:21 CEST, Jerry DeLisle <jvdelisle@charter.net> wrote:
>>On 10/13/2016 08:16 AM, Janne Blomqvist wrote:
>
>>>
>>> Regtested on x86_64-pc-linux-gnu. Ok for trunk?
>>>
>>
>>Yes, OK, clever! Thanks!
>
> Is 32 something a typical program uses?

Probably not. Then again, wasting a puny 32 bytes vs. the time it
takes to do one or two extra realloc+copy operations when opening that
many files?

> I'd have started at 8 and had not doubled but += 16 fwiw.

I can certainly start at a smaller value like 8 or 16, but I'd like to
keep the multiplicative increase in order to get O(log(N))
reallocs+copys rather than O(N) when increasing the size.

-- 
Janne Blomqvist

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH, libfortran] PR 48587 Newunit allocator
  2016-10-13 15:16 [PATCH, libfortran] PR 48587 Newunit allocator Janne Blomqvist
  2016-10-13 17:16 ` Jerry DeLisle
  2016-10-13 20:08 ` Jerry DeLisle
@ 2016-10-14 21:48 ` Janne Blomqvist
  2016-10-18  9:10 ` Steven Bosscher
  3 siblings, 0 replies; 10+ messages in thread
From: Janne Blomqvist @ 2016-10-14 21:48 UTC (permalink / raw)
  To: fortran, gcc-patches; +Cc: Janne Blomqvist

Currently GFortran newer reuses unit numbers allocated with NEWUNIT=,
instead having a simple counter that is decremented each time such a
unit is opened.  For a long running program which repeatedly opens
files with NEWUNIT= and closes them, the counter can wrap around and
cause an abort.  This patch replaces the counter with an allocator
that keeps track of which units numbers are allocated, and can reuse
them once they have been deallocated.  Since operating systems tend to
limit the number of simultaneous open files for a process to a
relatively modest number, a relatively simple approach with a linear
scan through an array suffices.  Though as a small optimization there
is a low water indicator keeping track of the index for which all unit
numbers below are already allocated.  This linear scan also ensures
that we always allocate the smallest available unit number.

2016-10-15  Janne Blomqvist  <jb@gcc.gnu.org>

	PR libfortran/48587
	* io/io.h (get_unique_unit_number): Remove prototype.
	(newunit_alloc): New prototype.
	* io/open.c (st_open): Call newunit_alloc.
	* io/unit.c (newunits,newunit_size,newunit_lwi): New static
	variables.
	(GFC_FIRST_NEWUNIT): Rename to NEWUNIT_START.
	(next_available_newunit): Remove variable.
	(get_unit): Call newunit_alloc, don't try to create negative
	external unit.
	(close_unit_1): Call newunit_free.
	(close_units): Free newunits array.
	(get_unique_number): Remove function.
	(newunit_alloc): New function.
	(newunit_free): New function.
	* io/transfer.c (data_transfer_init): Check for invalid unit
	number.

testsuite ChangeLog:

2016-10-15  Janne Blomqvist  <jb@gcc.gnu.org>

	PR libfortran/48587
	* gfortran.dg/negative_unit2.f90: New testcase.

Regtested on x86_64-pc-linux-gnu.

Version 2 of the patch. Compared to v1:
* Check for invalid unit number in transfer.c:data_transfer_init and
  unit.c:get_unit.
* Add testcase to check invalid unit number.
* Reduce initial size of newunits array from 32 to 16.
---
 gcc/testsuite/gfortran.dg/negative_unit2.f90 |   9 +++
 libgfortran/io/io.h                          |   5 +-
 libgfortran/io/open.c                        |   2 +-
 libgfortran/io/transfer.c                    |  10 ++-
 libgfortran/io/unit.c                        | 108 +++++++++++++++++++++------
 5 files changed, 107 insertions(+), 27 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/negative_unit2.f90

diff --git a/gcc/testsuite/gfortran.dg/negative_unit2.f90 b/gcc/testsuite/gfortran.dg/negative_unit2.f90
new file mode 100644
index 0000000..e7fb85e
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/negative_unit2.f90
@@ -0,0 +1,9 @@
+! { dg-do run }
+! Test case submitted by Dominique d'Humieres
+program negative_unit2
+  integer :: i, j
+  ! i should be <= NEWUNIT_FIRST in libgfortran/io/unit.c
+  i = -100
+  write(unit=i,fmt=*, iostat=j) 10
+  if (j == 0) call abort
+end program negative_unit2
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index ea93fba..aaacc08 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -715,8 +715,9 @@ internal_proto (finish_last_advance_record);
 extern int unit_truncate (gfc_unit *, gfc_offset, st_parameter_common *);
 internal_proto (unit_truncate);
 
-extern GFC_INTEGER_4 get_unique_unit_number (st_parameter_common *);
-internal_proto(get_unique_unit_number);
+extern int newunit_alloc (void);
+internal_proto(newunit_alloc);
+
 
 /* open.c */
 
diff --git a/libgfortran/io/open.c b/libgfortran/io/open.c
index d074b02..2e7163d 100644
--- a/libgfortran/io/open.c
+++ b/libgfortran/io/open.c
@@ -812,7 +812,7 @@ st_open (st_parameter_open *opp)
   if ((opp->common.flags & IOPARM_LIBRETURN_MASK) == IOPARM_LIBRETURN_OK)
     {
       if ((opp->common.flags & IOPARM_OPEN_HAS_NEWUNIT))
-	opp->common.unit = get_unique_unit_number(&opp->common);
+	opp->common.unit = newunit_alloc ();
       else if (opp->common.unit < 0)
 	{
 	  u = find_unit (opp->common.unit);
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 902c020..7696cca 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -2601,7 +2601,15 @@ data_transfer_init (st_parameter_dt *dtp, int read_flag)
 
   dtp->u.p.current_unit = get_unit (dtp, 1);
 
-  if (dtp->u.p.current_unit->s == NULL)
+  if (dtp->u.p.current_unit == NULL)
+    {
+      /* This means we tried to access an external unit < 0 without
+	 having opened it first with NEWUNIT=.  */
+      generate_error (&dtp->common, LIBERROR_BAD_OPTION,
+		      "Invalid unit number in statement");
+      return;
+    }
+  else if (dtp->u.p.current_unit->s == NULL)
     {  /* Open the unit with some default flags.  */
        st_parameter_open opp;
        unit_convert conv;
diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c
index 274b24b..41cd52f 100644
--- a/libgfortran/io/unit.c
+++ b/libgfortran/io/unit.c
@@ -29,6 +29,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #include "unix.h"
 #include <stdlib.h>
 #include <string.h>
+#include <assert.h>
 
 
 /* IO locking rules:
@@ -68,12 +69,34 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
    on it.  unlock_unit or close_unit must be always called only with the
    private lock held.  */
 
-/* Subroutines related to units */
 
-/* Unit number to be assigned when NEWUNIT is used in an OPEN statement.  */
-#define GFC_FIRST_NEWUNIT -10
+
+/* Table of allocated newunit values.  A simple solution would be to
+   map OS file descriptors (fd's) to unit numbers, e.g. with newunit =
+   -fd - 2, however that doesn't work since Fortran allows an existing
+   unit number to be reassociated with a new file. Thus the simple
+   approach may lead to a situation where we'd try to assign a
+   (negative) unit number which already exists. Hence we must keep
+   track of allocated newunit values ourselves. This is the purpose of
+   the newunits array. The indices map to newunit values as newunit =
+   -index + NEWUNIT_FIRST. E.g. newunits[0] having the value true
+   means that a unit with number NEWUNIT_FIRST exists. Similar to
+   POSIX file descriptors, we always allocate the lowest (in absolute
+   value) available unit number.
+ */
+static bool *newunits;
+static int newunit_size; /* Total number of elements in the newunits array.  */
+/* Low water indicator for the newunits array. Below the LWI all the
+   units are allocated, above and equal to the LWI there may be both
+   allocated and free units. */
+static int newunit_lwi;
+static void newunit_free (int);
+
+/* Unit numbers assigned with NEWUNIT start from here.  */
+#define NEWUNIT_START -10
+
+
 #define NEWUNIT_STACK_SIZE 16
-static GFC_INTEGER_4 next_available_newunit = GFC_FIRST_NEWUNIT;
 
 /* A stack to save previously used newunit-assigned unit numbers to
    allow them to be reused without reallocating the gfc_unit structure
@@ -81,6 +104,7 @@ static GFC_INTEGER_4 next_available_newunit = GFC_FIRST_NEWUNIT;
 static gfc_saved_unit newunit_stack[NEWUNIT_STACK_SIZE];
 static int newunit_tos = 0; /* Index to Top of Stack.  */
 
+
 #define CACHE_SIZE 3
 static gfc_unit *unit_cache[CACHE_SIZE];
 gfc_offset max_offset;
@@ -551,7 +575,7 @@ get_unit (st_parameter_dt *dtp, int do_create)
       if ((dtp->common.flags & IOPARM_DT_HAS_UDTIO) != 0)
 	{
 	  dtp->u.p.unit_is_internal = 1;
-	  dtp->common.unit = get_unique_unit_number (&dtp->common);
+	  dtp->common.unit = newunit_alloc ();
 	  unit = get_gfc_unit (dtp->common.unit, do_create);
 	  set_internal_unit (dtp, unit, kind);
 	  fbuf_init (unit, 128);
@@ -567,7 +591,7 @@ get_unit (st_parameter_dt *dtp, int do_create)
 	    }
 	  else
 	    {
-	      dtp->common.unit = get_unique_unit_number (&dtp->common);
+	      dtp->common.unit = newunit_alloc ();
 	      unit = xcalloc (1, sizeof (gfc_unit));
 	      fbuf_init (unit, 128);
 	    }
@@ -579,8 +603,12 @@ get_unit (st_parameter_dt *dtp, int do_create)
   dtp->u.p.unit_is_internal = 0;
   dtp->internal_unit = NULL;
   dtp->internal_unit_desc = NULL;
-  unit = get_gfc_unit (dtp->common.unit, do_create);
-  return unit;
+  /* For an external unit with unit number < 0 creating it on the fly
+     is not allowed, such units must be created with
+     OPEN(NEWUNIT=...).  */
+  if (dtp->common.unit < 0)
+    return get_gfc_unit (dtp->common.unit, 0);
+  return get_gfc_unit (dtp->common.unit, do_create);
 }
 
 
@@ -734,6 +762,9 @@ close_unit_1 (gfc_unit *u, int locked)
   free_format_hash_table (u);
   fbuf_destroy (u);
 
+  if (u->unit_number <= NEWUNIT_START)
+    newunit_free (u->unit_number);
+
   if (!locked)
     __gthread_mutex_unlock (&u->lock);
 
@@ -788,6 +819,9 @@ close_units (void)
 	free (newunit_stack[newunit_tos].unit->s);
 	free (newunit_stack[newunit_tos--].unit);
       }
+
+  free (newunits);
+
 #ifdef HAVE_FREELOCALE
   freelocale (c_locale);
 #endif
@@ -885,25 +919,53 @@ finish_last_advance_record (gfc_unit *u)
   fbuf_flush (u, u->mode);
 }
 
+
 /* Assign a negative number for NEWUNIT in OPEN statements or for
    internal units.  */
-GFC_INTEGER_4
-get_unique_unit_number (st_parameter_common *common)
+int
+newunit_alloc (void)
 {
-  GFC_INTEGER_4 num;
-
-#ifdef HAVE_SYNC_FETCH_AND_ADD
-  num = __sync_fetch_and_add (&next_available_newunit, -1);
-#else
   __gthread_mutex_lock (&unit_lock);
-  num = next_available_newunit--;
-  __gthread_mutex_unlock (&unit_lock);
-#endif
-  /* Do not allow NEWUNIT numbers to wrap.  */
-  if (num > GFC_FIRST_NEWUNIT)
+  if (!newunits)
     {
-      generate_error (common, LIBERROR_INTERNAL, "NEWUNIT exhausted");
-      return 0;
+      newunits = xcalloc (16, 1);
+      newunit_size = 16;
     }
-  return num;
+
+  /* Search for the next available newunit.  */
+  for (int ii = newunit_lwi; ii < newunit_size; ii++)
+    {
+      if (!newunits[ii])
+        {
+          newunits[ii] = true;
+          newunit_lwi = ii + 1;
+	  __gthread_mutex_unlock (&unit_lock);
+          return -ii + NEWUNIT_START;
+        }
+    }
+
+  /* Search failed, bump size of array and allocate the first
+     available unit.  */
+  int old_size = newunit_size;
+  newunit_size *= 2;
+  newunits = xrealloc (newunits, newunit_size);
+  memset (newunits + old_size, 0, old_size);
+  newunits[old_size] = true;
+  newunit_lwi = old_size + 1;
+    __gthread_mutex_unlock (&unit_lock);
+  return -old_size + NEWUNIT_START;
+}
+
+
+/* Free a previously allocated newunit= unit number.  unit_lock must
+   be held when calling.  */
+
+static void
+newunit_free (int unit)
+{
+  int ind = -unit + NEWUNIT_START;
+  assert(ind >= 0 && ind < newunit_size);
+  newunits[ind] = false;
+  if (ind < newunit_lwi)
+    newunit_lwi = ind;
 }
-- 
2.7.4

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH, libfortran] PR 48587 Newunit allocator
  2016-10-14 20:41     ` Janne Blomqvist
@ 2016-10-15  1:15       ` Bernhard Reutner-Fischer
  0 siblings, 0 replies; 10+ messages in thread
From: Bernhard Reutner-Fischer @ 2016-10-15  1:15 UTC (permalink / raw)
  To: Janne Blomqvist; +Cc: Jerry DeLisle, Fortran List, GCC Patches

On 14 October 2016 22:41:25 CEST, Janne Blomqvist <blomqvist.janne@gmail.com> wrote:
>On Fri, Oct 14, 2016 at 8:01 PM, Bernhard Reutner-Fischer
><rep.dot.nop@gmail.com> wrote:
>> On 13 October 2016 22:08:21 CEST, Jerry DeLisle
><jvdelisle@charter.net> wrote:
>>>On 10/13/2016 08:16 AM, Janne Blomqvist wrote:
>>
>>>>
>>>> Regtested on x86_64-pc-linux-gnu. Ok for trunk?
>>>>
>>>
>>>Yes, OK, clever! Thanks!
>>
>> Is 32 something a typical program uses?
>
>Probably not. Then again, wasting a puny 32 bytes vs. the time it
>takes to do one or two extra realloc+copy operations when opening that
>many files?

Every reallocated I'm aware of uses pools.

>
>> I'd have started at 8 and had not doubled but += 16 fwiw.
>
>I can certainly start at a smaller value like 8 or 16, but I'd like to

Yes please.

>keep the multiplicative increase in order to get O(log(N))
>reallocs+copys rather than O(N) when increasing the size.

Bike-shedding but if she's going to use that many units O(log(N)) will be nothing compared to the expected insn storm to follow. Inc by max(initial value, 64, let's say - short of double initial value - is still overestimated IMHO.
Thanks for taking care of this either way.
Cheers

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH, libfortran] PR 48587 Newunit allocator
  2016-10-13 15:16 [PATCH, libfortran] PR 48587 Newunit allocator Janne Blomqvist
                   ` (2 preceding siblings ...)
  2016-10-14 21:48 ` Janne Blomqvist
@ 2016-10-18  9:10 ` Steven Bosscher
  2016-10-18 10:14   ` Janne Blomqvist
  3 siblings, 1 reply; 10+ messages in thread
From: Steven Bosscher @ 2016-10-18  9:10 UTC (permalink / raw)
  To: Janne Blomqvist; +Cc: fortran, GCC Patches

On Thu, Oct 13, 2016 at 5:16 PM, Janne Blomqvist wrote:
> +static bool *newunits;

You could make this a bitmap (like sbitmap). A bit more code but makes
a potentially quadratic search (when opening many units) less time
consuming.

Ciao!
Steven

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH, libfortran] PR 48587 Newunit allocator
  2016-10-18  9:10 ` Steven Bosscher
@ 2016-10-18 10:14   ` Janne Blomqvist
  0 siblings, 0 replies; 10+ messages in thread
From: Janne Blomqvist @ 2016-10-18 10:14 UTC (permalink / raw)
  To: Steven Bosscher; +Cc: fortran, GCC Patches

On Tue, Oct 18, 2016 at 12:09 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Thu, Oct 13, 2016 at 5:16 PM, Janne Blomqvist wrote:
>> +static bool *newunits;
>
> You could make this a bitmap (like sbitmap). A bit more code but makes
> a potentially quadratic search (when opening many units) less time
> consuming.

I did think about that, yes, but decided that it wasn't worth the
extra complexity since

a) The OS typically limits the number of fd's per process to a
relatively small number (typically 1024 by default).

b) For better or worse, in libgfortran a unit is a quite big
structure, not to mention the 8 kB buffer. So obsessing over wasting
an extra 7 bits per unit seemed pointless.

c) Due to the newunit_lwi, in many scenarios it should be able to skip
scanning over, if not all then at least most of, the in-use units. Of
course, it's possible to design a scenario which defeats the lwi, but,
is that something real software does? And even if it does, due to a)
above I think the effect would be quite modest anyway.



-- 
Janne Blomqvist

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH, libfortran] PR 48587 Newunit allocator
@ 2016-10-13 21:00 Dominique d'Humières
  0 siblings, 0 replies; 10+ messages in thread
From: Dominique d'Humières @ 2016-10-13 21:00 UTC (permalink / raw)
  To: blomqvist.janne; +Cc: fortran, gcc-patches List

With the patch, the following code

integer :: i, j
i = -10
write(unit=i,fmt=*, iostat=j) 10
print *, j
end

fails at run time with

Assertion failed: (ind >= 0 && ind < newunit_size), function newunit_free, file ../../../work/libgfortran/io/unit.c, line 966.

Without the patch the output is 5002.

TIA

Dominique

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-10-18 10:14 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-13 15:16 [PATCH, libfortran] PR 48587 Newunit allocator Janne Blomqvist
2016-10-13 17:16 ` Jerry DeLisle
2016-10-13 20:08 ` Jerry DeLisle
2016-10-14 17:02   ` Bernhard Reutner-Fischer
2016-10-14 20:41     ` Janne Blomqvist
2016-10-15  1:15       ` Bernhard Reutner-Fischer
2016-10-14 21:48 ` Janne Blomqvist
2016-10-18  9:10 ` Steven Bosscher
2016-10-18 10:14   ` Janne Blomqvist
2016-10-13 21:00 Dominique d'Humières

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).