[Rt-devel] Slow Ticket History (from rt-users)

[Tim Cutts]

Hi chaps,

Some of you may not have seen the discussion about slow display of tickets with large numbers of transactions, on rt-users.

I've found a fairly significant performance issue in the code, which causes it to run O(N^2) or possibly worse, with respect to the number of transactions in the ticket.  My patch converts this to O(N) (I think!).

It's certainly a lot faster than it was, on my RT setup.  Here's my post to rt-users - I'd appreciate comments on my changes in case I've accidentally done something which will toast my data, but I don't think I have, and I've been testing this all day.

On 6 Sep 2010, at 5:54 pm, rt-users-request at lists.bestpractical.com wrote:

> So far we've tried installing RT on different hardware, both 32 and 64bit versions of linux. RT is still very slow for long tickets. All the time is
> taken up by the perl/apache process maxing out a core of CPU.
> 
> We've even gone as far as trying to profile the code. We came up with this graph of where the time was going:
> 
> <TIMING.png>
> We then tried to go further into those functions but can't find a single smoking gun call that is taking all the time.
> 
> For example in a ticket that takes 22s to render approx 5 secs goes on these 2 lines:
> 
> File: Ticket/Elements/ShowHistory line: 100-103 version 3.8.8
> 
> 	my @trans_attachments = grep { $_->TransactionId == $Transaction->Id } @attachments;
> 
> 	grep { ($_->TransactionId == $Transaction->Id ) && ($trans_content->{$_->Id} = $_)  } @attachment_content;
> 

From what I can tell, the real problem here is repeated scanning of both the @attachments and @attachment_content arrays, which makes the execution speed of the ShowHistory element O(N^2) with respect to the number of transactions; painful, to say the least.

1)  The %$trans_content hash can be made up front in a single pass, for all the attachments in the ticket, turning this into an O(N) operation, rather than O(N^2)
2) The $trans_content variable is only used in one place; it's passed to ShowTransaction, where it is then passed on to ShowTransactionAttachments.
3)  In there, there are some errors which cause some autovivification of hash members which needn't happen.
4)  You can do much the same up-front calculation with $trans_attachments as well, so you don't have to keep grepping through it

I've now made those changes, and on a reasonably large ticket (216 transactions) it reduced the ticket rendering time on my system from 2.5 minutes to 34 seconds, which is a pretty good improvement, I think.

On a more extreme ticket, with 417 transactions, the 3.8.8 release code takes at least 20 minutes to render the ticket (I gave up waiting and stopped the page load after that long), so in fact it's considerably worse order execution time than I thought.  My patched code takes 2.2 minutes - still not brilliant but hey, this ticket is now renderable, which it was not before.

Just for some background, the hardware I'm running on:

RT database:  2 CPU virtual machine, 8GB RAM, MySQL, running on vSphere 4.1 on a 2.0 GHz E5504 Nehalem system
RT web server:  2 CPU virtual machine, 2GB RAM, on the same type of physical hardware as the database server

As far as I can tell, I have not semantically altered the code, but others may want to test more thoroughly.  I have not yet put this on my production web server - I cloned my web server VM and made my changes to the clone (God, I love VMs for this sort of thing!)

Regards,

Tim

Here is my patch to Ticket/Elements/ShowHistory:

--- /opt/rt3/share/html/Ticket/Elements/ShowHistory	2010-05-14 13:58:15.000000000 +0100
+++ /opt/rt3/local/html/Ticket/Elements/ShowHistory	2010-09-07 11:59:30.000000000 +0100
@@ -84,6 +84,10 @@
<%perl>
my @attachments = @{$Attachments->ItemsArrayRef()};
my @attachment_content = @{$AttachmentContent->ItemsArrayRef()};
+my $trans_content = {};
+map { $trans_content->{$_->TransactionId}->{$_->Id} = $_  } @attachment_content;
+my $trans_attachments = {};
+map { push (@{$trans_attachments->{$_->TransactionId}}, $_) } @attachments;

while ( my $Transaction = $Transactions->Next ) {
    my $skip = 0;
@@ -97,12 +101,6 @@

    $i++;

-    my @trans_attachments = grep { $_->TransactionId == $Transaction->Id } @attachments;
-
-    my $trans_content = {};
-    grep { ($_->TransactionId == $Transaction->Id ) && ($trans_content->{$_->Id} = $_)  } @attachment_content;
-
-   
    my $IsLastTransaction = 0;
    if ( $OldestFirst ) {
        $IsLastTransaction = $Transactions->IsLast;
@@ -118,7 +116,7 @@
              Transaction          => $Transaction,
              ShowHeaders          => $ShowHeaders,
              RowNum               => $i,
-              Attachments          => \@trans_attachments,
+              Attachments          => $trans_attachments->{$Transaction->id},
              AttachmentContent    => $trans_content,
              LastTransaction      => $IsLastTransaction
 );

and here's the patch to ShowTransactionAttachments (both files need to be patched):

--- /opt/rt3/share/html/Ticket/Elements/ShowTransactionAttachments	2010-05-14 13:58:15.000000000 +0100
+++ /opt/rt3/local/html/Ticket/Elements/ShowTransactionAttachments	2010-09-07 11:04:53.000000000 +0100
@@ -189,8 +189,10 @@
        {

            my $content;
-            if ( $AttachmentContent->{ $message->id } ) {
-                $content = $AttachmentContent->{ $message->id }->Content;
+	    my ($transaction_id, $message_id) = ($Transaction->id, $message->id);
+            if ( exists($AttachmentContent->{ $transaction_id }) &&
+		 exists($AttachmentContent->{ $transaction_id }->{$message_id}) ) {
+                $content = $AttachmentContent->{ $transaction_id }->{ $message_id }->Content;
            }
            else {
                $content = $message->Content;




-- 
 The Wellcome Trust Sanger Institute is operated by Genome Research 
 Limited, a charity registered in England with number 1021457 and a 
 company registered in England with number 2742969, whose registered 
 office is 215 Euston Road, London, NW1 2BE.