Re: [patch, libfortran] PR31501 libgfortran internal unit I/O performance issues

[Jerry DeLisle <jvdelisle at verizon dot net>]

FX Coudert wrote:
> Hi Jerry,
>
> I'm sorry for stepping in just now, when you said you're going to 
> commit; I had your patch on my "review list" since you posted it. It's 
> all fair and good, but I don't understand completely why the part below 
> is safe. IIUC, you're going from "reading internal unit buffers one char 
> at a time" to "reading *length characters immediately". Does that mean 
> that the assumption in the comment "readlen may be modified inside 
> mem_allow_r_at" is just wrong? Because later, you don't reuse the value 
> of readlen, so I expect that if it has changed, we're loosing some 
> information here.
>
> Thanks for your help,
> FX
>
>
>
> > Index: transfer.c
> > ===================================================================
> > *** transfer.c    (revision 124025)
> > --- transfer.c    (working copy)
> > *************** read_sf (st_parameter_dt *dtp, int *leng
> > *** 164,181 ****
> >         return base;
> >       }
> >
> >     readlen = 1;
> >     n = 0;
> >
> >     do
> >       {
> > -       if (is_internal_unit (dtp))
> > -     {
> > -       /* readlen may be modified inside salloc_r if
> > -          is_internal_unit (dtp) is true.  */
> > -       readlen = 1;
> > -     }
> > -
> >         q = salloc_r (dtp->u.p.current_unit->s, &readlen);
> >         if (q == NULL)
> >       break;
> > --- 164,182 ----
> >         return base;
> >       }
> >
> > +   if (is_internal_unit (dtp))
> > +     {
> > +       readlen = *length;
> > +       q = salloc_r (dtp->u.p.current_unit->s, &readlen);
> > +       memcpy (p, q, readlen);
> > +       goto done;
> > +     }
> > +
> >     readlen = 1;
> >     n = 0;
> >
> >     do
> >       {
> >         q = salloc_r (dtp->u.p.current_unit->s, &readlen);
> >         if (q == NULL)
> >       break;
> > *************** read_sf (st_parameter_dt *dtp, int *leng
> > *** 244,249 ****
> > --- 245,252 ----
> >         dtp->u.p.sf_seen_eor = 0;
> >       }
> >     while (n < *length);
> > +
> > +  done:
> >     dtp->u.p.current_unit->bytes_left -= *length;
> >
> >     if ((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0)

read_sf is called from read_block.  The tests for EOR or EOF are done before the 
call to read_sf.  With internal units, we always know the length of the 
character string which defines the record length.  This is stored in bytes left. 
 There is no '\n' or other terminating character we are looking for to know 
when we are at the end of the record, unlike external files.  When we complete 
the read, we know we are done.

It is true that readlen is modified for each call to salloc_r.  If we are inside 
the loop, reading one byte at a time, the readlen has to be set for each pass. 
However, by lifting this code out of the loop, readlen gets set once as passed 
from read_block before the call to sallloc_r and that is sufficient.

Repeating myself, We are safe because we always test to make sure the bytes 
requested is fewer or equal to the bytes available before the call to read_sf.

I will add that the way we are using salloc_r, I think we can greatly simplify 
the code inside mem_alloc_r_at.  There are several variables that are simply 
always zero. (That will be on the next round here)

Jerry