267822 - Disable install.php redirects (and downloadcounts)

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[Wolf [:wolf]]

There's alot of problems with the downloadcount code and while it may be
unpopular to disable it, and also disable the install.php redirection
pass-through in the process, there's a few good reasons for doing so.

- Firefox 1.0 looming in the very near future, the server performance on
iguana.mozilla.org will need to be as high as it can be to handle the load.
Using workers to process redirections and wait for queries to finish instead of
pointing  directly to the files creates unneeded overhead and blocks other requests.

- The downloadcounts are already disabled for themes because installTrigger is
not optional there. Theme counts are based on downloads-only, not installs.

- The downloadcount codepath has alot of queries in it, upon investigatino of
the latest performance problem with iguana, it seems stuck queries from the
downloadcount codepath were likely causing the server to peak at it's max of
1000 simultaneous requests and not drop, (server load averages of 30 or more,
highest I observed was 80, entirely too high). Disabling the
/extensions/install.php at line 46 and having it redirect w/o processing the
queries, allowed it to recover. If this reoccurs during the peak of the release,
the performance of the server will drop to nothing, particularly for https
requests. This'll affect the webservice and pluginfinder.

- The download counts aren't particularly accurate anyway, because of various
issues over time, and point #2.

I've already got a patch to move the site over to installTrigger calls, from Bug
265135. My plan is to land that patch,  along with a patch to install.php
that'll make it return and redirect if hit at line 46. This'll have the added
benefits of making the install process more friendly as it'll show the
theme/extension name and version, etc, and was already planned for update-beta
anyway. For update-beta, this will be extended further by unbreaking
downloadcounts with a better performing method probably called by client-side
JS. (XMLHttpRequest was suggested.)

Comment 1

[Wolf [:wolf]]

Attachment: Disable DownloadCount code in install.php (returns at line 46)

Comment 2

[Wolf [:wolf]]

Attachment: Updated Patch for using InstallTrigger for Install Links

Updated patch from the patch from Bug 265135. This changes all the Install
Links to use InstallTrigger and not just install.php.

Comment 3

[Wolf [:wolf]]

Patches Checked in and live.

Comment 4

[Giorgio Maone [:ma1]]

Hi Wolf.
You introduced a syntax error, I believe you forgot the semicolon (;) after

header("Location: $uri")

at line 46.

Hope you're reading 'cause I can't reopen.

Comment 5

[Wolf [:wolf]]

Reopening per regression in Comment #4

Comment 6

[Wolf [:wolf]]

Attachment: Regression Fix. Include the ; at the end of the line.

Oops. I thought I tested that. heh.

Comment 7

[Wolf [:wolf]]

Fixed.

Comment 8

[Giorgio Maone [:ma1]]

Hi Wolf,
I don't think redirection itself is such a big performance hitter, but actually
there's some heavy DB synchronous processing that makes it a serious bottleneck.  

Do you mind if I review your SQL code to see if there's room for optimization,
for instance using delayed and batch MySQL statements?

Which version of MySQL is in use?
Are table types MyISAM or InnoDB?

Thank you for your attention.

Comment 9

[Giorgio Maone [:ma1]]

Hi Wolf, I wrote 

> Which version of MySQL is in use?
> Are table types MyISAM or InnoDB?

Silly me, I've found creation scripts:

Server version: 4.0.18
All the tables Type=InnoDB.

Comment 10

[Giorgio Maone [:ma1]]

Attachment: Test script, shows 300% performance improvement using indexes on t_downloads

Hi Wolf.
Is a 3x speed improvement enough?

I've tried many things (using transactions, collapsing two updates and two
deletes that worked on the same tables...), but the most effective seems also
the simplest: adding two missing indexes in table t_donwloads.

alter table t_downloads 
  add index i_downloads_type(type), 
  add index i_downloads_date(date);

I don't know  your numbers, but in my test environment loaded with 

600 t_main records
3000 t_version records
90000 (about) t_downloads records

It performed over 300% better than original setup with many concurrent requests
(including redirection overhead).

In the "duplicate click" case the speed improvement is near 10x!

If you're interested I've attached my test script, which forks several wget
instances to test the concurrency load, and this is a sample output:

=============================================
Summary: NOIDX 60 secs / IDX: 18 secs
Total requests: 100
Duplicate requests: 1 (1%)
NOIDX / IDX execution time ratio: 333.33%


Hope it helps to bring DownloadCount alive again :-)

Comment 11

[Wolf [:wolf]]

(In reply to comment #10)
Thanks! Your test tables are actually bigger than the update live tables. (main
is 359, versions 1,060) :-) I've added the indexes you suggested as part of the
work on Bug 251050. Which is where the re-activation/improvement work is going
on. :-)