424187 - Increase ySlow score for all AMO pages to A

Status: RESOLVED FIXED

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

Description

[Ryan Doherty (:rdoherty)]

Just did a quick ySlow run for the AMO homepage, F!

Here's a rundown of the score. It won't be easy to fix everything, but it's possible :)

Performance Grade: F (57)
F	1. Make fewer HTTP requests
This page has 4 external JavaScript files.
This page has 5 external StyleSheets.
This page has 19 CSS background images.

F	2. Use a CDN

F	3. Add an Expires header
These components do not have a far future Expires header:
(all files)

F	4. Gzip components
These components are not gzipped:
https://preview.addons.mozilla.org/js/__utm.js
https://preview.addons.mozilla.org/en-US/firefox/pages/js_constants.js
https://preview.addons.mozilla.org/js/jquery-compressed.js
https://preview.addons.mozilla.org/js/addons.js

A	5. Put CSS at the top

C	6. Put JS at the bottom
4 external scripts were found in the document HEAD. Could they be moved lower in the page?

A	7. Avoid CSS expressions

n/a	8. Make JS and CSS external

A	9. Reduce DNS lookups

B	10. Minify JS
https://preview.addons.mozilla.org/js/__utm.js
https://preview.addons.mozilla.org/js/addons.js

A	11. Avoid redirects
A	12. Remove duplicate scripts
A	13. Configure ETags

Comment 1

[Fred Wenzel [:wenzel]]

For the JS files, we already use a compressed version of jquery, we could do the same for the others, while keeping the uncompressed ones around for development of course ;). We'd need to remember re-compressing them for every change to the source files though.

Comment 2

[Ryan Doherty (:rdoherty)]

(In reply to comment #1)

We use a packed version of jquery, which is actually bigger than a gzipped version of the minified one. See bug 422815.

We'll need a build script to create a release version of AMO that can conctatenate all CSS & JS, minify and change src urls in templates. 


See http://developer.yahoo.com/performance/rules.html & http://developer.yahoo.net/blog/archives/2008/03/yahoos_latest_p.html

Comment 3

[Ryan Doherty (:rdoherty)]

http://stevesouders.com/cuzillion/

Neat tool to test various page performance issues.

Comment 4

[Brian Krausz]

Taking this, will begin work on it next week.

Comment 5

[Ryan Doherty (:rdoherty)]

FYI we are up to a C for the homepage. Gzipping and adding addons.mozilla.org as a CDN are the reasons. 

addons.mozilla.org can be considered a semi-cdn because we have 3 locations for our datacenters around the world. 

Comment 6

[Brian Krausz]

My dev environment has 1 and 10 brought up to an A (will be checking in after code review today).

That makes the only things not A's:
3. Add an Expires header (F)
6. Put JS at the bottom (C)

I will investigate if 6 can be done.

For 3 we can set future expires headers for our images, as long as we remember to change file names if we change files.  I already have a build script written to compress/minify JS & CSS, we can set future expires for these in conjunction with a build-set timestamp or revision #.  Example:

We push a new stylesheet, r12345.  Url to stylesheet is styles.min.css?12345
Next push, r23456 url is updated to styles.min.css?23456

The one issue is how/where to store this parameter.  Ideally this can be done as part of the build process Ryan's working on, where it dumps the revision to a file ala `define('REVISION', 12345);`

Opinions?

Comment 7

[Ryan Doherty (:rdoherty)]

(In reply to comment #6)
> My dev environment has 1 and 10 brought up to an A (will be checking in after
> code review today).

Sweet :)

> The one issue is how/where to store this parameter.  Ideally this can be done
> as part of the build process Ryan's working on, where it dumps the revision to
> a file ala `define('REVISION', 12345);`
> 
> Opinions?
> 

It can definitely be done via a build process. For now it could be something really simple that does the performance stuff, then checks with svn for the rev # and writes out/appends to a file with the revision #. 

As much as I want a more structured/complicated build tool, simple scripts may be enough for now :)

Comment 8

[Brian Krausz]

Everything's been committed: r16721.  Will look into having it stages/QAed.  A change needs to be deployed to staging/production config files to enable js/css compression, will file IT bug.

Comment 9

[Justin Scott [:fligtar]]

http://viewvc.svn.mozilla.org/vc/addons/branches/performance/site/app/config/constants.php?limit_changes=0&r1=16721&r2=16720&pathrev=16721

SITE_URL is already defined and isn't in that format -- I'm guessing you meant to have SITE_STATE.

Kinda nervous about all these global changes without review.

Comment 10

[Brian Krausz]

Correct, SITE_STATE (I don't recall specifying a file).

rdoherty and I had an offline review since there was an image and a lot of minor things involved.  I'll attach a patch here for review and approval.

Comment 11

[Ryan Doherty (:rdoherty)]

(In reply to comment #9)

> 
> Kinda nervous about all these global changes without review.
> 

I reviewed them with Brian earlier today. 

Comment 12

[Brian Krausz]

Doh, just realized my checkin has all the info of the patch, nevermind :-P

Comment 13

[Justin Scott [:fligtar]]

I think you misunderstood my SITE_URL comment. SITE_URL is now defined twice in constants.php.

/**
 * Site URL default
 */
if (!defined('SITE_URL'))
    define('SITE_URL', 'http://addons.mozilla.org');

/**
 * Site State default
 */
if (!defined('SITE_URL'))
    define('SITE_URL', 'dev');

Comment 14

[Brian Krausz]

aha, my mistake.  All fixed: r16723.

Comment 15

[Brian Krausz]

Marking this complete...needs QA.  Here's the process for getting this up (just FYI):

1) QA, make sure I didn't screw anything up ;-)
2) Push to production.  This will enable my spriting, but the file concatenation requires a config change.
3) Once that's up, I'll file a bug for adding the config parameter to enable file concatenation to production and staging.

QA: please look over anything involving background graphics, layout, and javascript (primarily the bg graphics, that has the most change).

Thanks!

Comment 16

[Brian Krausz]

4) Ask IT to up cache time for js/css to years.

Comment 17

[Ryan Doherty (:rdoherty)]

1.1) Prioritize with next AMO release
1.2) Merge branch into trunk

Comment 18

[Brian Krausz]

Stephen should be looking at this today...forgot to remind him: in addition to checking background images and basic JS in english, can you check it in an rtl language such as he?

Thx!

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

Reopening; IE 6 isn't really as happy as it could be with this :-(

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: IE 6, Hebrew

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: IE 6, English (en-US)

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

http://bk-amo.khan.mozilla.org isn't functional right now for me to resume testing (I can reach the host, just am returned a blank page) -- Brian, can you take a look?

Comment 23

[Brian Krausz]

Strange, I haven't touched it since Saturday, and it was working then.  Could be a db issue, will investigate tomorrow.

Comment 24

[Brian Krausz]

Nevermind...mysql had crashed...restarted it.  Working fine now :)

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screenshot of "Advanced" tab background-fill fail

This is fine on production and preview, but in your perf instance, the "Advanced" tab for longer-length texts (such as in the [nl] locale) don't get their background completely filled.

Comment 26

[Stephen Donner [:stephend] Not actively reading bugmail]

Also, the add-ons featured in the "Recommended" pages (http://bk-amo.khan.mozilla.org/en-US/firefox/recommended) are missing their background fill too.

Ditto for http://bk-amo.khan.mozilla.org/en-US/firefox/browse/type:1/cat:all?show=20&exp=on&sort=newest, which also suffers from its bottom border image not being aligned right.

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screenshot of the bottom-border issue

Take a look at the "Chrome List" add-on's bottom-border image (mis)alignment.

Comment 28

[Brian Krausz]

Everything should be all prettied up now :-D

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

The issues in comment 6 / comment 7 are fixed.

In addition to Firefox 2 / 3, I tested:

* Safari 3.1.2
* Opera 9.51
* IE 7
* IE 6 [1]

Caveats: 

[1] IE 6 is still affected as it was in comment 21 / comment 22, but basic functionality still seems to be there; heard we're not holding up this for IE 6, since it still works, just doesn't look pretty.
[2] I didn't get a chance to test the theme browser view (https://addons.mozilla.org/en-US/firefox/browse/type:2/cat:all?sort=name), because the bk-amo.khan.mozilla.org instance has a weird DB dump/old code.

Anyway, with those couple issues noted, I think it looks good; nice work, Brian!

Comment 30

[Stephen Donner [:stephend] Not actively reading bugmail]

Argh, I meant "the issues in comment 26 / comment 27 are fixed."  -- sorry!

Comment 31

[Brian Krausz]

Boss-man...can you confirm that this screenshot of IE6 is ok, assuming functionality is still intact?

https://bugzilla.mozilla.org/attachment.cgi?id=329552

If that is OK I'll merge perf branch into trunk.

Thanks!

Comment 32

[Michael Morgan [:morgamic]]

I'm okay with it.

Comment 33

[Brian Krausz]

morgamic++

r17141, done

Comment 34

[Brian Krausz]

Forgot the tag

Comment 35

[Stephen Donner [:stephend] Not actively reading bugmail]

As our YSlow score has moved up but to only a "D" (1. went from F->D, and 2. dropped from B->C), I'm assuming we still need the config change on preview; oremj: can we get that implemented so I can re-run YSlow and verify before we push to prod?  Thanks!

Comment 36

[Brian Krausz]

Comments on parts of current score:

> D	1. Make fewer HTTP requests
> This page has 4 external JavaScript files.
> This page has 5 external StyleSheets.
> This page has 11 CSS background images.

These will be combined into 3 JS files and 1 stylesheet once config changes get pushed (bug 445869).  The images are a result of the new middle box not using my CSS.  I filed bug 445972 for this, I'll fix it tomorrow.

> F	2. Use a CDN

If you expand this tab you see an option for adding to the CDN list.  You can add m.o and m.c, since we have multiple clusters in different geographic regions, qualifying us as a mini-CDN.

> F	3. Add an Expires header

Once we get this stuff pushed I can file to add expires headers to *.js? and *.css?, but until then I can't

Everything else is an A, and the above changes will most likely bring these into A's.  A's for everyone!

Comment 37

[Ryan Doherty (:rdoherty)]

(In reply to comment #36)

> > F	3. Add an Expires header
> 
> Once we get this stuff pushed I can file to add expires headers to *.js? and
> *.css?, but until then I can't

We'll probably want to double-check we're not missing any css or js files. If we do we could accidentally send users files that never expire (until we change the url).

Comment 38

[Brian Krausz]

I was actually hoping they could do URL matching for .css? and .js?, that way CSS and JS files that we miss won't be cached.  Either way I'll look around for css/js that we missed.

Comment 39

[Ryan Doherty (:rdoherty)]

(In reply to comment #38)
> I was actually hoping they could do URL matching for .css? and .js?, that way
> CSS and JS files that we miss won't be cached.  Either way I'll look around for
> css/js that we missed.
> 

Ah, good point.

Comment 40

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: YSlow score of where we're currently at

Comment 41

[Stephen Donner [:stephend] Not actively reading bugmail]

So, we're good to go, looks like.  As mentioned in comment 36, we can't do F until we push to production, and the only other remaining score is 6. Put JS at the bottom, which I'm guessing we can't do for various reasons.

I'll verify this bug after we push to production.

Comment 42

[Stephen Donner [:stephend] Not actively reading bugmail]

Do we need a separate bug (IT bug?) for the expires headers stuff?  (Comment 36.)

Comment 43

[Brian Krausz]

Yes we do, see bug 448050

Comment 44

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #43)
> Yes we do, see bug 448050

Someone please cc me on that?