[Piglit] [PATCH 4/8] framework: Remove micro-optimization with no impact.

[Dylan Baker]

On Wednesday 18 September 2013 15:48:30 Eric Anholt wrote:
> Dylan Baker <baker.dylan.c at gmail.com> writes:
> > On Wednesday 18 September 2013 15:00:29 Eric Anholt wrote:
> >> We can compute these lists anyway.  Note how we don't really do much
> >> work to generate
> >> ---
> >> 
> >>  framework/summary.py | 59
> >> 
> >> ++++++++++++++++++++-------------------------------- 1 file changed, 23
> >> insertions(+), 36 deletions(-)
> >> 
> >> diff --git a/framework/summary.py b/framework/summary.py
> >> index 656efc0..8f6f5f7 100644
> >> --- a/framework/summary.py
> >> +++ b/framework/summary.py
> >> 
> >> @@ -397,19 +397,10 @@ class Summary:
> >>              # Create a list with all the test names in it
> >>              self.tests['all'] = set(self.tests['all']) | set(each.tests)
> >> 
> >> -    def __generate_lists(self, lists):
> >> 
> >> +    def __generate_lists(self):
> >>          """
> >>          Private: Generate the lists of changes, problems, regressions,
> >> 
> >> fixes, and skips
> >> -
> >> -        lists is a list contianing any of the following: changes,
> >> problems, -        skips, fixes (which will also generate regressions)
> >> -
> >> -        This method has different code paths to allow the exclusion of
> >> certain -        lists being generated. This is both useful for speeding
> >> up
> >> HTML -        generation when a page isn't needed (regressions with only
> >> one test -        file is provided), and for JUnit and text which only
> >> need
> >> a limited -        subset of these lists
> >> 
> >>          """
> >>          
> >>          def find_regressions(status):
> >>              """
> >> 
> >> @@ -441,31 +432,27 @@ class Summary:
> >>                  status_text.append(s)
> >>                  status.append(find_regressions(s))
> >> 
> >> -            if 'changes' in lists:
> >> -                # Check and append self.tests['changes']
> >> -                # A set cannot contain duplicate entries, so creating a
> >> set -                # out the list will reduce it's length to 1 if all
> >> entries -                # are the same, meaning it is not a change
> >> -                if len(set(status)) > 1:
> >> -                    self.tests['changes'].add(test)
> >> -
> >> -            if 'problems' in lists:
> >> -                for i in status_text:
> >> -                    if i not in ['pass', 'skip', 'special']:
> >> -                        self.tests['problems'].add(test)
> >> -
> >> -            if 'skipped' in lists:
> >> -                if 'skip' in status_text:
> >> -                    self.tests['skipped'].add(test)
> >> -
> >> -            if 'fixes' in lists:
> >> -                # Find both fixes and regressions, and append them to
> >> the
> >> -                # proper lists
> >> -                for i in xrange(len(status) - 1):
> >> -                    if status[i] < status[i + 1] and status[i] != 0:
> >> -                        self.tests['regressions'].add(test)
> >> -                    if status[i] > 1 and status[i + 1] == 1:
> >> -                        self.tests['fixes'].add(test)
> >> +            # Check and append self.tests['changes']
> >> +            # A set cannot contain duplicate entries, so creating a set
> >> +            # out the list will reduce it's length to 1 if all entries
> >> +            # are the same, meaning it is not a change
> >> +            if len(set(status)) > 1:
> >> +                self.tests['changes'].add(test)
> >> +
> >> +            for i in status_text:
> >> +                if i not in ['pass', 'skip', 'special']:
> >> +                    self.tests['problems'].add(test)
> >> +
> >> +            if 'skip' in status_text:
> >> +                self.tests['skipped'].add(test)
> >> +
> >> +            # Find both fixes and regressions, and append them to the
> >> +            # proper lists
> >> +            for i in xrange(len(status) - 1):
> >> +                if status[i] < status[i + 1] and status[i] != 0:
> >> +                    self.tests['regressions'].add(test)
> >> +                if status[i] > 1 and status[i + 1] == 1:
> >> +                    self.tests['fixes'].add(test)
> >> 
> >>      def __find_totals(self):
> >>          """
> >> 
> >> @@ -560,7 +547,7 @@ class Summary:
> >>          else:
> >>              pages = ['problems', 'skipped']
> >> 
> >> -        self.__generate_lists(pages)
> >> +        self.__generate_lists()
> >> 
> >>          # Index.html is a bit of a special case since there is index,
> >>          all,
> >> 
> >> and # alltests, where the other pages all use the same name. ie,
> > 
> > IIRC there is a pretty major performance difference as more tests are
> > added to the list, but if there isn't then could we just fold this back
> > into the constructor, as it was originally?
> 
> I tested with junit and a single results list of a full piglit run,
> which should highlight this "optimization" to the maximum extent
> possible as far as I can tell.

Um, junit doesn't run on this code path at all, it's runs completely out the 
the piglit-summary-junit.py file, and that was Jose's decision.

Practically thinking about I'll conceed that this optimization isn't worth the 
complexity it adds, since even if it works the way I believe it does it would 
be nearly impossible to actually hit a case that could benefit from it (a large 
list of test runs, that are all the same would be required). So, I'd like to 
see it just merged back into the constructor, but either way this patch is:
Reviewed-by: Dylan Baker <baker.dylan.c at gmail.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130919/7dbc4822/attachment.pgp>