Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Piwik UI faster: merge all Javascript and CSS files together, minify, cache the merged files on disk #660

Closed
mattab opened this issue Apr 15, 2009 · 32 comments
Assignees
Labels
Critical Indicates the severity of an issue is very critical and the issue has a very high priority. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Apr 15, 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](http://code.openx.org/repositories/changes/ox/www/admin/assets/combine/combine.php?rev=32194)

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](http://developer.yahoo.com/performance/rules.html#etags))
- uses [JMIN php implentation](http://code.google.com/p/jsmin-php/)
- OpenX implementation doesn’t use CSS minifier but we could add this feature: [CSS minifier in php](http://code.google.com/p/cssmin/). 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! :)

@mattab
Copy link
Member Author

mattab commented Apr 15, 2009

It is so much faster this way! It worked well on the test website at [divezone.net/bali](http://divezone.net/bali.htm).

@mattab
Copy link
Member Author

mattab commented Apr 15, 2009

see also very interesting read: [netflix lessons learned](http://billwscott.com/share/presentations/2008/stanford/HPWP-RealWorld.pdf)

@robocoder
Copy link
Contributor

@mattab
Copy link
Member Author

mattab commented May 22, 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
Copy link
Contributor

Attachment:
function.packerload.php

@robocoder
Copy link
Contributor

This plugin allows the template to output script/css/meta tags, wherever, and using an output filter, to relocate the tags to the <head>.

@anonymous-matomo-user
Copy link

Attachment:
piwik-script-consolidate.tar.gz

@anonymous-matomo-user
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

@robocoder
Copy link
Contributor

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

@mattab
Copy link
Member Author

mattab commented Jun 1, 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:

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
Copy link
Member

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

@julienmoumne
Copy link
Member

Attachment:
piwik_660.pdf

@julienmoumne
Copy link
Member

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.

@julienmoumne
Copy link
Member

Attachment:
(#660).patch

@mattab
Copy link
Member Author

mattab commented Jun 21, 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
Copy link
Member

Attachment:
(#660).2.patch

@julienmoumne
Copy link
Member

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
Copy link
Member

New version of the patch includes :

  • updates to 0.6.3
  • css conflicts resolved (including [1444])

@julienmoumne
Copy link
Member

Attachment:
(#660).3.patch

@robocoder
Copy link
Contributor

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
Copy link
Member Author

mattab commented Jun 29, 2010

Committed patch in [2392] - Thanks JulienM!

@mattab
Copy link
Member Author

mattab commented Jun 29, 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
Copy link
Member Author

mattab commented Jun 29, 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
Copy link
Member Author

mattab commented Jun 29, 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
Copy link
Member Author

mattab commented Jul 1, 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
Copy link
Member Author

mattab commented Jul 13, 2010

Great job Julien!!

@mattab
Copy link
Member Author

mattab commented Jul 21, 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
Copy link
Member

(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
Copy link
Contributor

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

@anonymous-matomo-user
Copy link

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

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical Indicates the severity of an issue is very critical and the issue has a very high priority. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

No branches or pull requests

4 participants