| Message ID | 1476371788-6740-1-git-send-email-blomqvist.janne@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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
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
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.
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; }