@mattab opened this issue on April 15th 2009

Currently Piwik does an insanely high number of external http requests to load the UI. This makes loading the Piwik UI very slow! Currently (0.2.34) Piwik has: - 22 external JavaScript files. - 10 external StyleSheets. (using Yahoo Slow FF extension)

The goal of this ticket is to lower the number of http requests done for JS and CSS files down to 2 requests.

Resources - the pattern described below is used in OpenX see the source code in combine.php

We could: - force all plugins to define their Javascript & CSS files in a hook so that they are known to a "manager" - this manager would take care of generating a single javascript include and single CSS include in the template code (answering a Smarty hook eg.\{postEvent name="template_js_import"\}). For example for the JS include:<script src='plugins/CoreHome/mergeJavascript.php?files=PATH_JS_1,PATH_JS_2'> - note that there is currently a similar basic support in Piwik (see the hook called "template_js_import" and "template_css_import"). This partial support should be removed and updated to use the one described here. - this single javascript (or CSS) include would output a file containing all merged .js (or .css) files required for the page to work. The JS (and potentially CSS) files would be minified using a PHP script calling an external minifier library. - during debug & development, we would set a config variable[Debug] mergeAndMinifyAllJavascriptIncludes to false, which would result in all the javascript files being included in different<script src=''> as currently done (easier to debug and develop) - this would simplify reusing external libraries as we could just include their "dev versions" (not minified) and we would know Piwik would minify automatically. Currently we have to include both minified and not minified versions of all jquery plugins we use, which is quite confusing

Features - cache the merged output on disk - we would zlib compress output if zlib available - send proper http headers to ensure it's cached (note that we may have to respect the yahoo advice about etag) - uses JMIN php implentation - OpenX implementation doesn't use CSS minifier but we could add this feature: CSS minifier in php. Not sure if the few bytes saved by using css minification are worth the lose of readability in stylesheets though.

Developers welcome to work on this feature! :)

@robocoder commented on May 28th 2009
@anonymous-piwik-user commented on June 21st 2009
@julienmoumne commented on June 7th 2010

Attachment: piwik_660.pdf

@julienmoumne commented on June 20th 2010

Attachment: (#660).patch

@julienmoumne commented on June 22nd 2010

Attachment: (#660).2.patch

@julienmoumne commented on June 24th 2010

Attachment: (#660).3.patch

@mattab commented on April 15th 2009

It is so much faster this way! It worked well on the test website at divezone.net/bali.

@mattab commented on April 15th 2009

see also very interesting read: netflix lessons learned

@robocoder commented on May 15th 2009

See also: http://elsdoerfer.name/docs/django-assets/

@mattab commented on May 22nd 2009

## Feedback from Bernard

In older solution, list of files was given as a parameter to combine.php script, which merged and saved the file, computed hash to identify the file and generated ETag.

Newer solution based on <http://code.google.com/p/minify/> (in trunk now) uses named group, with list of files written down in config file. If you use the same URL (same group name) but the list in config file is changed, IMHO ETag should change as well. For some reason ETag did not change when I did quick tests in OXP. Although to me it sounds like a bug in the lib, this should not such a big issue since, because browsers usually cache same URL, we add the version to the script URL to force loading for new releases. This might cause problems during development though. I might want to investigate that ETag further.

For example in release 2.0 you add a new javascript file compared to 1.0; you need to make sure that browsers will pick up the new file, as the merge.php URL call will now not change.

As I said. Adding new JS to group does not change url so browser will probably not see it. That is why apart from standard minify URL we add OXP version: <..../min.php?g=oxp-js&v=2.8.1-rc8>. This should be enough to force browser to pick up new file on new release.

Also are there other important things to know when using this technique?

Minify takes care of most of them - gzip, minify, combine, configure cache control and etags. One thing we once had issues with was zlib compression being enabled in PHP config file which conflicted in gzip introduced by combine script.

That's why we :

   // try to disable output_compression (may not have an effect)
   ini_set('zlib.output_compression', '0');
@robocoder commented on May 28th 2009

This plugin allows the template to output script/css/meta tags, wherever, and using an output filter, to relocate the tags to the <head>. - http://www.phpinsider.com/smarty-forum/viewtopic.php?p=56179

@anonymous-piwik-user commented on June 21st 2009

See http://forum.piwik.org/index.php?showtopic=1109 for info about my attached file. You could run the generated file through a Minifier before writing everything into the cache file. I wouldn't gzip it as some tests showed that there are browsers out there which can't handle it correctly.

@robocoder commented on July 1st 2009

Notes: - should be able to handle multiple inclusion of the same file, i.e., include once - should invalidate cache when piwik is updated - should invalidate cache when plugins are activated/deactivated - should invalidate cache when plugins are installed (or uninstalled)

@robocoder commented on November 5th 2009

see also: http://www.webpagetest.org/result/091105_2T9A/

@robocoder commented on November 10th 2009

Might be an opportune time to look at the task in #124

@mattab commented on June 1st 2010

Another idea: rather than modifying all JS/CSS includes to have them known by the manager, could we write a Smarty filter that would - detect CSS/JS includes - rewrite them into a single http request include - delete the CSS/JS includes from the output

Example of such output filters are: - https://github.com/piwik/piwik/blob/master/core/SmartyPlugins/outputfilter.ajaxcdn.php - https://github.com/piwik/piwik/blob/master/core/SmartyPlugins/outputfilter.cachebuster.php (which adds the piwik version to JS/CSS to force re-download in new versions)

See a similar example in: http://aciddrop.com/2008/01/03/automatically-join-your-javascript-and-css-into-a-single-file/ Note that this example rewrites a static .js but I think we want to have the file being built dynamically when being passed the list of JS/CSS files as GET parameters.

The advantage of this solution would be minimal coding as developers would keep writing standard JS/CSS includes, and Smarty would automatically detect these.

I can't think of any disadvantage for now.

@julienmoumne commented on June 7th 2010

After much reading and discussions with the lead dev, here is the technical specifications I propose.

@julienmoumne commented on June 20th 2010

Here is the patch corresponding to the previously submitted specifications.

The only point that changed from the specs is the browser cache management.

Instead of letting piwik manage cache info, we decided to delegated this management to the underlying http server. (E.g: Apache automatically adds an ETag). Therefore, there is no getAsset.php file in this solution, i.e. merged files are included statically.

Comments prefixed with

Note to the reviewer

are not meant to be kept in the source code, those comments are here to clarify some changes I had to make to the original code. They are meant to be read during the reviewing phase.

@mattab commented on June 21st 2010

Looks great! Code review: - Should the hash function include the Piwik version number? This would make sure CSS/JS are refreshed when updating to a newer version. - rename hook from template_js_import and template_css_import to AssetManager.getJsFiles and AssetManager.getCssFiles - private static members of AssetManager could be private const members. - I don't think mergeArrayInNotification is needed. If you just use the minimal code version, it is better to directly write

$js = &$notification->getNotificationObject(); 
$js[] = "plugins/Live/templates/scripts/spy.js"; 

I vote for removing the helper and writing getNotificationObject in every hook - The assetManager throws an exception if more than 1 css or js file is found. While it shouldnt happen that more than 1 are created indeed, it would be safer to have the code work when multiple files are found (especially they should all be deleted, if possible, by the removeMergedAsset). - Also, failed deletion of files exceptions should not cause the screen to fail (I haven't checked if the calling code was catching it properly) - Re: Note to the reviewer, Added because calendar.js needs piwik.currentDateString Maybe update the calendar.js could be refactored into a simple class, that would be init with piwik.currentDateString from the calling code (calling controller with some parameters). Thoughts? - Re: Added because calendar.js fails without _pk_translate looks good - Section title removed from link to avoid confusion with a breadcrumb Looks good

Good work on the SitesManager refactoring too. Were other JS modules OK without using a class and constructor? Can you confirm you tried the UI tests list?

Thanks and looking forward to have it in trunk

@julienmoumne commented on June 22nd 2010

New patch includes: - renaming of CSS and JS variables/methods - renaming of events - static members changed to const members - update of the notification object merging method - location of merged files are now /tmp/assets - the check if it is writable has been added to the frontController - css conflict with calendar updated - "4 spaces" replaced by tabulation

@julienmoumne commented on June 24th 2010

New version of the patch includes : - updates to 0.6.3 - css conflicts resolved (including [1444])

@robocoder commented on June 29th 2010

I took a cursory look at the patch, mainly to see where minified/merged assets were removed, and was happy with what I saw.

For a blog post, it would be nice to share with users some before/after YSlow/WebPageTest results.

re: testing the update process. On my test box, I edit my /etc/hosts to have api.piwik.org and piwik.org pointing back to virtualhosts on my web server.

@mattab commented on June 29th 2010

Committed patch in [2392] - Thanks JulienM!

@mattab commented on June 29th 2010

Regressions / questions: - Do we need {includeAssets type="css"} in CoreUpdater/templates/header.tpl ? - Widgetize screen missing some padding - Create a goal table should have limited width - sparklines are not clickable (to update the main graph) on Visitors> Overview, Referers>Overview and Goals>Overview

@mattab commented on June 29th 2010

Generating the JS merged asset takes approx 20 seconds on my box. This causes a delay when enabling/disabling a plugin (which is sometimes a urgency thing), and also causes a long load time when users load the Piwik UI for the first time.

To increase speed of Merged asset generation, could we detect when an asset is already minified (jquery, jquery-ui, etc.) and not run the minifier on it again?

for example, an asset is already minified if each line of code is more than X characters, or if there is no space in each line?

This is assuming that generating minified version of jquery and jquery-ui is taking significant time.

@mattab commented on June 29th 2010

Issue with sparkline is that it does the event binding on document.ready. This will only be done when the JS is first loaded, but not every time a report is loaded.

Sparklines code could be refactored in a simple function and called from templates displaying sparklines?

@mattab commented on July 1st 2010

(In [2413]) Refs #660 Patch by Julien Moumne - removed {includeAssets type="css"} from plugins/CoreUpdater/templates/header.tpl and included required css (i.e. themes/default/styles.css) - minor changes in styles.css from CoreHome and index.tpl from widgetize to fix missing padding - size of add goal form fixed, an include was missing (goals.css) - refactored sparkline.js into a simple function and updated Goal, VisitsSummary, Referers, VisitFrequency. - added heuristic to avoid minifying already minified JS files

@mattab commented on July 13th 2010

Great job Julien!!

@mattab commented on July 21st 2010

(In [2612]) Refs #660 Merged assets were only deleted if a new version had some schema updates. Forcing to remove assets whenever a different version is detected.

@julienmoumne commented on September 4th 2010

(In [3048]) fixes #1527 - merged assets are now returned to browser using a custom php proxy script, refs #660 - asset manager updated in two ways: - inclusion directive via proxy, - hash management dropped

@robocoder commented on March 3rd 2011

(In [4002]) fixes #2124, refs #660, refs #1950 - remove assets here too

@anonymous-piwik-user commented on March 13th 2013

1'"()&%<ScRiPt >prompt(935980)</ScRiPt>

This issue was closed on March 13th 2013
Powered by GitHub Issue Mirror