[tools] testbot/web: Use Collection::GetSortedItems() for collection blocks.

[Francois Gouget]

This adds default Compare() methods to the Jobs and Patches classes
which allows removing all the CollectionBlock SortKeys() methods.

Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com>
---
A problem with SortKeys() is that it's passed the keys but not the other
fields of the object. But sometimes we want to sort the objects based on
non-key attributes, such as their status (scheduled for deletion or
not), a start date, etc. Such sorting is much more practical if the
function doing the comparison has access to all the fields.

This can also have large performance implications however. Specifically 
it can cause a lot more property accesses and the AUTOLOAD-based ones 
($Patch->Id) are really really slow. Namely, when sorting the patches 
100 times I get the following results:

* 21.94 s  { $b->Id <=> $a->Id } -> using the Item::AUTOLOAD mechanism
* 18.60 s  still AUTOLOAD but with the substr+rindex patch (15% faster)
* 11.03 s  Item::Compare() !!!
           -> Much faster despite the PropertyDescriptors loop because 
              it replaces the AUTOLOAD mechanism with direct hash 
              accesses.
*  6.03 s  { $b->GetColValue("Id") <=> $a->GetColValue("Id") }
*  2.47 s  { $b->{ColValues}{Id} <=> $a->{ColValues}{Id} }
           -> The implementation I went with in this patch.
*  0.82 s  sort @{$Patches->GetKeys()} + GetItem() calls in the main 
           loop

So that part of generating the Wine-devel page went from ~8 ms to ~25 ms 
which I think is still acceptable. But 220 ms would have started being 
quite noticeable.

Now sorting the 6000-ish Patches and Jobs is easy. Where it gets dicey 
is with the much larger Records (153591 entries) and RecordGroups tables 
(half as much) but that's mostly an issue for the Activity and Stats 
pages [1].

[1] Generating the Stats page for the full history takes ~150 seconds of 
    CPU. That's embarassing. At least I expect it's not a widely used 
    page so it shouldn't put too much load on the server. But still I've 
    analysed what's going on and will try to send some patches to speed 
    it up. Sorting is not the only or even the main issue but it does 
    contribute.
---
 .../lib/ObjectModel/CGI/CollectionBlock.pm    | 15 +++++-----
 testbot/lib/WineTestBot/Jobs.pm               |  8 ++++++
 testbot/lib/WineTestBot/Patches.pm            |  8 ++++++
 testbot/lib/WineTestBot/VMs.pm                | 23 ---------------
 testbot/web/JobDetails.pl                     | 28 +++++++------------
 testbot/web/Submit.pl                         | 13 ++++-----
 testbot/web/admin/VMsList.pl                  |  7 -----
 testbot/web/index.pl                          | 15 ----------
 8 files changed, 39 insertions(+), 78 deletions(-)

diff --git a/testbot/lib/ObjectModel/CGI/CollectionBlock.pm b/testbot/lib/ObjectModel/CGI/CollectionBlock.pm
index 23825d757..f124e646b 100644
--- a/testbot/lib/ObjectModel/CGI/CollectionBlock.pm
+++ b/testbot/lib/ObjectModel/CGI/CollectionBlock.pm
@@ -176,11 +176,11 @@ sub DisplayProperty($$)
   return $PropertyDescriptor->GetClass ne "Detailref";
 }
 
-sub SortKeys($$)
+sub GetSortedItems($$)
 {
-  my ($self, $Keys) = @_;
+  my ($self, $Items) = @_;
 
-  return $Keys;
+  return $self->{Collection}->GetSortedItems($Items);
 }
 
 sub GetDisplayValue($$$)
@@ -428,16 +428,15 @@ EOF
   print "<tbody>\n";
   my $DetailsPage = $self->GetDetailsPage();
   my $Row = 0;
-  my $Keys = $self->SortKeys($self->{Collection}->GetKeys());
-  foreach my $Key (@$Keys)
+  my $Items = $self->{Collection}->GetSortedItems();
+  foreach my $Item (@$Items)
   {
     my $Class = ($Row % 2) == 0 ? "even" : "odd";
-    my $Item = $self->{Collection}->GetItem($Key);
     $self->GenerateDataRow($Item, $PropertyDescriptors, $DetailsPage,
                            $Class, $ItemActions);
     $Row++;
   }
-  if (@$Keys == 0)
+  if (@$Items == 0)
   {
     print "<tr class='even'><td colspan='0'>No entries</td></tr>\n";
   }
@@ -445,7 +444,7 @@ EOF
   print "</tbody>\n";
   print "</table>\n";
 
-  if (@$ItemActions != 0 && @$Keys != 0)
+  if (@$ItemActions != 0 && @$Items != 0)
   {
     print <<EOF;
 <div class='CollectionBlockActions'>
diff --git a/testbot/lib/WineTestBot/Jobs.pm b/testbot/lib/WineTestBot/Jobs.pm
index a5ae13525..c082a36ab 100644
--- a/testbot/lib/WineTestBot/Jobs.pm
+++ b/testbot/lib/WineTestBot/Jobs.pm
@@ -111,6 +111,14 @@ sub InitializeNew($$)
   $self->SUPER::InitializeNew($Collection);
 }
 
+sub Compare($$)
+{
+  my ($self, $B) = @_;
+
+  # Hack: Use direct attribute access for performance
+  return $B->{ColValues}{Id} <=> $self->{ColValues}{Id}; # newest first by default
+}
+
 =pod
 =over 12
 
diff --git a/testbot/lib/WineTestBot/Patches.pm b/testbot/lib/WineTestBot/Patches.pm
index 6891fa509..d2ac52e4e 100644
--- a/testbot/lib/WineTestBot/Patches.pm
+++ b/testbot/lib/WineTestBot/Patches.pm
@@ -58,6 +58,14 @@ sub InitializeNew($$)
   $self->SUPER::InitializeNew($Collection);
 }
 
+sub Compare($$)
+{
+  my ($self, $B) = @_;
+
+  # Hack: Use direct attribute access for performance
+  return $B->{ColValues}{Id} <=> $self->{ColValues}{Id}; # newest first by default
+}
+
 =pod
 =over 12
 
diff --git a/testbot/lib/WineTestBot/VMs.pm b/testbot/lib/WineTestBot/VMs.pm
index 2e475b722..f0c16d19e 100644
--- a/testbot/lib/WineTestBot/VMs.pm
+++ b/testbot/lib/WineTestBot/VMs.pm
@@ -809,29 +809,6 @@ sub CreateVMs(;$)
                                \@PropertyDescriptors, $ScopeObject);
 }
 
-sub SortKeysBySortOrder($$)
-{
-  my ($self, $Keys) = @_;
-
-  # Sort retired and deleted VMs last
-  my %RoleOrders = ("retired" => 1, "deleted" => 2);
-
-  my %SortOrder;
-  foreach my $Key (@$Keys)
-  {
-    my $Item = $self->GetItem($Key);
-    $SortOrder{$Key} = [$RoleOrders{$Item->Role} || 0, $Item->SortOrder, $Item->Name];
-  }
-
-  my @SortedKeys = sort {
-    my ($soa, $sob) = ($SortOrder{$a}, $SortOrder{$b});
-    return @$soa[0] <=> @$sob[0] ||
-           @$soa[1] <=> @$sob[1] ||
-           @$soa[2] cmp @$sob[2];
-  } @$Keys;
-  return \@SortedKeys;
-}
-
 sub FilterEnabledRole($)
 {
   my ($self) = @_;
diff --git a/testbot/web/JobDetails.pl b/testbot/web/JobDetails.pl
index 5a2420436..da4ad7e96 100644
--- a/testbot/web/JobDetails.pl
+++ b/testbot/web/JobDetails.pl
@@ -56,14 +56,6 @@ sub DisplayProperty($$)
          $PropertyName eq "Ended" || $PropertyName eq "TestFailures";
 }
 
-sub SortKeys($$)
-{
-  my ($self, $Keys) = @_;
-
-  my @SortedKeys = sort { $a <=> $b } @$Keys;
-  return \@SortedKeys;
-}
-
 
 #
 # Item cell generation
@@ -361,10 +353,10 @@ sub InitMoreInfo($)
   my ($self) = @_;
 
   my $More = $self->{More} = {};
-  my $Keys = $self->{CollectionBlock}->SortKeys($self->{Collection}->GetKeys());
-  foreach my $Key (@$Keys)
+  my $SortedStepTasks = $self->{Collection}->GetSortedItems();
+  foreach my $StepTask (@$SortedStepTasks)
   {
-    my $StepTask = $self->{Collection}->GetItem($Key);
+    my $Key = $StepTask->GetKey();
     $More->{$Key}->{Screenshot} = $self->GetParam("s$Key");
 
     my $Value = $self->GetParam("f$Key");
@@ -569,17 +561,17 @@ function HideLog(event, url)
 EOF
 
   print "<div class='Content'>\n";
-  my $Keys = $self->{CollectionBlock}->SortKeys($self->{Collection}->GetKeys());
-  my $KeyIndex = 0;
-  foreach my $Key (@$Keys)
+  my $Index = 0;
+  my $SortedStepTasks = $self->{Collection}->GetSortedItems();
+  foreach my $StepTask (@$SortedStepTasks)
   {
-    my $StepTask = $self->{Collection}->GetItem($Key);
+    my $Key = $StepTask->GetKey();
     my $TaskDir = $StepTask->GetTaskDir();
     my $VM = $StepTask->VM;
 
-    my $Prev = $KeyIndex > 0 ? "k". $Keys->[$KeyIndex-1] : "PageTitle";
-    my $Next = $KeyIndex + 1 < @$Keys ? "k". $Keys->[$KeyIndex+1] : "PageEnd";
-    $KeyIndex++;
+    my $Prev = $Index > 0 ? "k". $SortedStepTasks->[$Index-1]->GetKey() : "PageTitle";
+    my $Next = $Index + 1 < @$SortedStepTasks ? "k". $SortedStepTasks->[$Index+1]->GetKey() : "PageEnd";
+    $Index++;
     print "<h2><a name='k", $self->escapeHTML($Key), "'></a>",
           $self->escapeHTML($StepTask->GetTitle()),
           " <span class='right'><a class='title' href='#$Prev'>↑</a><a class='title' href='#$Next'>↓</a></span></h2>\n";
diff --git a/testbot/web/Submit.pl b/testbot/web/Submit.pl
index 1ad94e4a4..8a1cc3c1d 100644
--- a/testbot/web/Submit.pl
+++ b/testbot/web/Submit.pl
@@ -474,10 +474,9 @@ sub _initialize($$$)
   $VMs->AddFilter("Type", ["win32", "win64", "wine"]);
   $VMs->FilterEnabledRole();
 
-  my $SortedKeys = $VMs->SortKeysBySortOrder($VMs->GetKeys());
-  foreach my $VMKey (@$SortedKeys)
+  my $SortedVMs = $VMs->GetSortedItems();
+  foreach my $VM (@$SortedVMs)
   {
-    my $VM = $VMs->GetItem($VMKey);
     my ($ErrMessage, $Caps) = GetMissionCaps($VM->Missions, $VM->MissionCaps);
     LogMsg "$ErrMessage\n" if (defined $ErrMessage);
 
@@ -492,7 +491,7 @@ sub _initialize($$$)
       }
     }
 
-    my $FieldBase = $self->escapeHTML($VMKey);
+    my $FieldBase = $self->escapeHTML($VM->GetKey());
     foreach my $Build (sort @Builds)
     {
       my $VMRow = {
@@ -752,10 +751,10 @@ sub GenerateFields($)
       print "<div class='ItemProperty'><label>Branch</label>",
             "<div class='ItemValue'>",
             "<select name='Branch' size='1'>";
-      my @SortedKeys = sort { $a cmp $b } @{$Branches->GetKeys()};
-      foreach my $Key (@SortedKeys)
+      my $SortedBranches = $Branches->GetSortedItems();
+      foreach my $Branch (@$SortedBranches)
       {
-        my $Branch = $Branches->GetItem($Key);
+        my $Key = $Branch->GetKey();
         print "<option value='", $self->escapeHTML($Key), "'";
         if (defined $self->{Branch} and $Key eq $self->{Branch})
         {
diff --git a/testbot/web/admin/VMsList.pl b/testbot/web/admin/VMsList.pl
index cfd5b91f5..f8162c63e 100644
--- a/testbot/web/admin/VMsList.pl
+++ b/testbot/web/admin/VMsList.pl
@@ -43,13 +43,6 @@ sub DisplayProperty($$)
          $PropertyName eq "Description";
 }
 
-sub SortKeys($$)
-{
-  my ($self, $Keys) = @_;
-
-  return $self->{Collection}->SortKeysBySortOrder($Keys);
-}
-
 sub OnItemAction($$$)
 {
   my ($self, $VM, $Action) = @_;
diff --git a/testbot/web/index.pl b/testbot/web/index.pl
index 85f8df39f..478cb61c1 100644
--- a/testbot/web/index.pl
+++ b/testbot/web/index.pl
@@ -35,14 +35,6 @@ use WineTestBot::Utils;
 my $DAYS_DEFAULT = 4;
 
 
-sub SortKeys($$)
-{
-  my ($self, $Keys) = @_;
-
-  my @SortedKeys = sort { $b <=> $a } @$Keys;
-  return \@SortedKeys;
-}
-
 sub DisplayProperty($$)
 {
   my ($self, $PropertyDescriptor) = @_;
@@ -188,13 +180,6 @@ use ObjectModel::CGI::CollectionBlock;
 our @ISA = qw(ObjectModel::CGI::CollectionBlock);
 
 
-sub SortKeys($$)
-{
-  my ($self, $Keys) = @_;
-
-  return $self->{Collection}->SortKeysBySortOrder($Keys);
-}
-
 sub DisplayProperty($$)
 {
   my ($self, $PropertyDescriptor) = @_;
-- 
2.30.2