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@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) = @_;