[tools] testbot/web: Use Collection::GetSortedItems() for collection blocks.
Francois Gouget
fgouget at codeweavers.com
Wed Apr 13 12:44:53 CDT 2022
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 at 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
More information about the wine-devel
mailing list