| Message ID | bce47860-bff0-7120-5842-9a7652fd4d24@charter.net |
|---|---|
| State | New |
| Headers | show |
Am 29.05.2017 um 01:16 schrieb Jerry DeLisle: > Hi all, > > The problem here is that we never set the err return to LIBERROR_END in all cases. For the example case we are detecting the EOF condition inside the read_integer procedure and it gets acted on correctly at higher levels in the code. Consequently in the big loop over the array where we call list_formatted_read_scalar, we never returned an error code so we never exited the loop early. > > The patch tests for the EOF first locally as before, but then returns the error flags set in dtp->common.flags which are set globally in the individual read procedures whene hit_eof is called. > > Regression tested on x86_64. I have added a test case which will check the execution time of the loop. The previous results of the REAd were correct, just took a long time on large arrays. > Seems to work as advertised. With this small patch, I see a tremendous speedup for array reads. The implied-do variant gets slightly slower (~10%), but the array variant now takes 0.002s independent of the size of "m", compared to some dozens of seconds without this patch! Concerning your test case: Your timeout of 2s is dangerously close to the timings of really fast boxes without this patch, so I would lower this value. I guess even on really slow ARM boxes or some-such this test case finishes in some few tenth of seconds, at worst. Or, as the new behavior seems to be independent of the m setting, just bump the constant m by a factor 10 or 100. So you are sure no big iron can pass this test without your patch being applied. Thanks a bunch! Manfred > OK for trunk? > > Regards, > > Jerry > > 2017-05-28 Jerry DeLisle <jvdelisle@gcc.gnu.org> > > PR libgfortran/35339 > * list_read.c.c (list_formatted_read_scala): Set the err return > value to the common.flags error values.
Hi Jerry, > Regression tested on x86_64. I have added a test case which will check > the execution time of the loop. The previous results of the REAd were > correct, just took a long time on large arrays. > > OK for trunk? OK. It might be good if you followed Manfred's suggestion and turned down the timeout to something like 0.5 seconds. Thanks for the patch! I would also consider backporting, the speedup is just so large. What do others think? Regards Thomas
On 05/29/2017 09:51 AM, Thomas Koenig wrote: > Hi Jerry, > >> Regression tested on x86_64. I have added a test case which will check the >> execution time of the loop. The previous results of the REAd were correct, >> just took a long time on large arrays. >> >> OK for trunk? > > OK. > > It might be good if you followed Manfred's suggestion and turned > down the timeout to something like 0.5 seconds. > > Thanks for the patch! > > I would also consider backporting, the speedup is just so > large. What do others think? > > Regards > > Thomas Committed. A gcc/testsuite/gfortran.dg/read_5.f90 M gcc/testsuite/ChangeLog M libgfortran/ChangeLog M libgfortran/io/list_read.c Committed r248577 Thanks, Jerry
diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c index 6c00d11b..b6cd6670 100644 --- a/libgfortran/io/list_read.c +++ b/libgfortran/io/list_read.c @@ -2298,11 +2298,16 @@ list_formatted_read_scalar (st_parameter_dt *dtp, bt type, void *p, free_saved (dtp); cleanup: + /* err may have been set above from finish_separator, so if it is set + trigger the hit_eof. The hit_eof will set bits in common.flags. */ if (err == LIBERROR_END) { free_line (dtp); hit_eof (dtp); } + /* Now we check common.flags for any errors that could have occurred in + a READ elsewhere such as in read_integer. */ + err = dtp->common.flags & IOPARM_LIBRETURN_MASK; fbuf_flush_list (dtp->u.p.current_unit, LIST_READING); return err; }