| Message ID | 20141002164739.GA25260@gate.crashing.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 02, 2014 at 11:47:39AM -0500, Segher Boessenkool wrote: > On Wed, Sep 24, 2014 at 10:54:57AM -0400, Andrew MacLeod wrote: > > Is this suppose to be resolved now? I'm still seeing some issues with a > > branch cut from mainline from yesterday. > > Confirmed. The following patch works for me, and Andrew has tested it > as well. The comment it removes isn't valid before the patch either. > > Okay for mainline? > > > Segher > > > 2014-10-02 Segher Boessenkool <segher@kernel.crashing.org> > > contrib/ > * dg-extract-results.py (output_variation): Always sort if do_sum. Ok, thanks. > --- a/contrib/dg-extract-results.py > +++ b/contrib/dg-extract-results.py > @@ -495,15 +495,7 @@ class Prog: > key = attrgetter ('name')): > sys.stdout.write ('Running ' + harness.name + ' ...\n') > if self.do_sum: > - # Keep the original test result order if there was only > - # one segment for this harness. This is needed for > - # unsorted.exp, which has unusual test names. Otherwise > - # sort the tests by test filename. If there are several > - # subtests for the same test filename (such as 'compilation', > - # 'test for excess errors', etc.) then keep the subtests > - # in the original order. > - if len (harness.segments) > 1: > - harness.results.sort() > + harness.results.sort() > for (key, line) in harness.results: > sys.stdout.write (line) > else: Jakub
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Wed, Sep 24, 2014 at 10:54:57AM -0400, Andrew MacLeod wrote: >> Is this suppose to be resolved now? I'm still seeing some issues with a >> branch cut from mainline from yesterday. > > Confirmed. The following patch works for me, and Andrew has tested it > as well. The comment it removes isn't valid before the patch either. I get the impression from a short dismissal like that that this script is pretty hated :-(. Remember that originally the script was trying to make the result of combining separate .sum files the same as the .sum file you'd get for -j1. As Jakub said upthread, that's a lost cause with the new approach to parallel testing, but I think the comment was valid while matching -j1 was still a goal. Thanks, Richard
On Thu, Oct 02, 2014 at 06:46:19PM +0100, Richard Sandiford wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Wed, Sep 24, 2014 at 10:54:57AM -0400, Andrew MacLeod wrote: > >> Is this suppose to be resolved now? I'm still seeing some issues with a > >> branch cut from mainline from yesterday. > > > > Confirmed. The following patch works for me, and Andrew has tested it > > as well. The comment it removes isn't valid before the patch either. > > I get the impression from a short dismissal like that that this script > is pretty hated :-(. Remember that originally the script was trying to No, it is certainly appreciated that it speeded up the processing. > make the result of combining separate .sum files the same as the .sum > file you'd get for -j1. As Jakub said upthread, that's a lost cause > with the new approach to parallel testing, but I think the comment was > valid while matching -j1 was still a goal. I'm sorry for invalidating those assumptions. Indeed, before my recent changes, all tests for the same testcase name were run serially by the same job. If we wanted to preserve that property, we could e.g. store the results of gcc_parallel_test_run_p in some tcl array with testcase as the key, and after the if { $gcc_runtest_parallelize_enable == 0 } { return 1 } test add a test if we've been asked about a particular testcase already, just return what we've returned before. Perhaps accompany that with lowering the granularity (e.g. from 10 to 5). Jakub
On Thu, Oct 02, 2014 at 06:46:19PM +0100, Richard Sandiford wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Wed, Sep 24, 2014 at 10:54:57AM -0400, Andrew MacLeod wrote: > >> Is this suppose to be resolved now? I'm still seeing some issues with a > >> branch cut from mainline from yesterday. > > > > Confirmed. The following patch works for me, and Andrew has tested it > > as well. The comment it removes isn't valid before the patch either. > > I get the impression from a short dismissal like that that this script > is pretty hated :-(. I meant that it isn't valid currently; it was valid before the parallelisation patches. It would be nice if we could reconstruct the original order somehow. Without this patch the order is different every run though, and that makes comparing testresults unworkable. Segher
On 10/02/2014 02:14 PM, Segher Boessenkool wrote: > On Thu, Oct 02, 2014 at 06:46:19PM +0100, Richard Sandiford wrote: >> Segher Boessenkool <segher@kernel.crashing.org> writes: >>> On Wed, Sep 24, 2014 at 10:54:57AM -0400, Andrew MacLeod wrote: >>>> Is this suppose to be resolved now? I'm still seeing some issues with a >>>> branch cut from mainline from yesterday. >>> Confirmed. The following patch works for me, and Andrew has tested it >>> as well. The comment it removes isn't valid before the patch either. >> I get the impression from a short dismissal like that that this script >> is pretty hated :-(. > I meant that it isn't valid currently; it was valid before the parallelisation > patches. It would be nice if we could reconstruct the original order somehow. > Without this patch the order is different every run though, and that makes > comparing testresults unworkable. > > Doesn't this patch make it always sort? And that should mean that -j1 will be the same as -JN again... ? it won't be the same order as before the patches... but I doubt that is important... not that I'm aware of anyway. Andrew Andrew
Jakub Jelinek <jakub@redhat.com> writes: >> make the result of combining separate .sum files the same as the .sum >> file you'd get for -j1. As Jakub said upthread, that's a lost cause >> with the new approach to parallel testing, but I think the comment was >> valid while matching -j1 was still a goal. > > I'm sorry for invalidating those assumptions. Indeed, before my recent > changes, all tests for the same testcase name were run serially by the same > job. If we wanted to preserve that property, we could e.g. store the > results of gcc_parallel_test_run_p in some tcl array with testcase as the > key, and after the > if { $gcc_runtest_parallelize_enable == 0 } { > return 1 > } > test add a test if we've been asked about a particular testcase already, > just return what we've returned before. Perhaps accompany that with > lowering the granularity (e.g. from 10 to 5). That sounds like it'd help if we have any lingering cases where the text after the PASS: etc. isn't unique, since otherwise it's probably unpredictable which one comes first in the combined summary (as it was when the test order was still keyed only off filename). OTOH I suppose we should just fix those tests so that the name is unique. Also, now that even partial RUNTESTFLAGS-based testuite runs can be fully parallelised (very nice, thanks), what -j1 did is probably no longer relevant anyway. Thanks, Richard
On Oct 4, 2014, at 3:32 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote:
> we should just fix those tests so that the name is unique.
Yes. This is good in all sorts of ways.
diff --git a/contrib/dg-extract-results.py b/contrib/dg-extract-results.py index fafd38e..7db5e64 100644 --- a/contrib/dg-extract-results.py +++ b/contrib/dg-extract-results.py @@ -495,15 +495,7 @@ class Prog: key = attrgetter ('name')): sys.stdout.write ('Running ' + harness.name + ' ...\n') if self.do_sum: - # Keep the original test result order if there was only - # one segment for this harness. This is needed for - # unsorted.exp, which has unusual test names. Otherwise - # sort the tests by test filename. If there are several - # subtests for the same test filename (such as 'compilation', - # 'test for excess errors', etc.) then keep the subtests - # in the original order. - if len (harness.segments) > 1: - harness.results.sort() + harness.results.sort() for (key, line) in harness.results: sys.stdout.write (line) else: