Opened 5 years ago

Closed 4 years ago

Last modified 13 months ago

#660 closed New feature (fixed)

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

Reported by: matt Owned by: JulienM
Priority: critical Milestone: Piwik 0.6.4
Component: UI - UX (AngularJS, twig, less) Keywords:
Cc: Sensitive: no

Description (last modified by matt)

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! :)

Attachments (6)

function.packerload.php (1.8 KB) - added by vipsoft 5 years ago.
piwik-script-consolidate.tar.gz (2.2 KB) - added by mbirth 5 years ago.
piwik_660.pdf (37.1 KB) - added by JulienM 4 years ago.
(#660).patch (81.4 KB) - added by JulienM 4 years ago.
(#660).2.patch (79.9 KB) - added by JulienM 4 years ago.
(#660).3.patch (81.7 KB) - added by JulienM 4 years ago.

Download all attachments as: .zip

Change History (52)

comment:1 Changed 5 years ago by matt (mattab)

  • Description modified (diff)

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

comment:2 Changed 5 years ago by matt (mattab)

  • Type changed from Bug to New feature

see also very interesting read: netflix lessons learned

comment:4 Changed 5 years ago by matt (mattab)

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');

comment:5 Changed 5 years ago by vipsoft (robocoder)

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

Changed 5 years ago by mbirth

comment:6 Changed 5 years ago by mbirth

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.

comment:7 Changed 5 years ago by dzirt

comment:8 Changed 5 years ago by Zithromax

comment:9 Changed 5 years ago by dzirt

comment:10 Changed 5 years ago by matt (mattab)

  • Milestone changed from 4- Stable release to Features requests - after Piwik 1.0

comment:11 Changed 5 years ago by vipsoft (robocoder)

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)

comment:12 Changed 5 years ago by bobolok

comment:13 Changed 5 years ago by alivenk

comment:14 Changed 5 years ago by domtop

comment:15 Changed 5 years ago by koteiko

comment:16 Changed 5 years ago by Smith

comment:17 Changed 5 years ago by Smith

comment:18 Changed 4 years ago by vipsoft (robocoder)

  • Milestone changed from Features requests - after Piwik 1.0 to 1 - Piwik 0.5
  • Owner set to vipsoft
  • Sensitive unset

comment:20 Changed 4 years ago by vipsoft (robocoder)

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

comment:21 Changed 4 years ago by vipsoft (robocoder)

  • Milestone changed from 1 - Piwik 0.5 to 1 - Piwik 0.5.1

comment:22 Changed 4 years ago by vipsoft (robocoder)

  • Owner vipsoft deleted

comment:23 Changed 4 years ago by matt (mattab)

  • Priority changed from major to normal

comment:24 Changed 4 years ago by vipsoft (robocoder)

  • Owner set to vipsoft

comment:25 Changed 4 years ago by vipsoft (robocoder)

  • Milestone changed from 1 - Piwik 0.5.5 to Features requests - after Piwik 1.0
  • Owner vipsoft deleted

comment:26 Changed 4 years ago by matt (mattab)

  • Priority changed from normal to critical

comment:27 Changed 4 years ago by matt (mattab)

  • Description modified (diff)

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.

comment:28 Changed 4 years ago by matt (mattab)

  • Owner set to JulienM

comment:29 Changed 4 years ago by JulienM (JulienMoumne)

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

comment:30 Changed 4 years ago by JulienM (JulienMoumne)

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.

Changed 4 years ago by JulienM (JulienMoumne)

comment:31 Changed 4 years ago by matt (mattab)

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

comment:32 Changed 4 years ago by pinkgranite

comment:33 Changed 4 years ago by JulienM (JulienMoumne)

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

comment:34 Changed 4 years ago by JulienM (JulienMoumne)

New version of the patch includes :

  • updates to 0.6.3

comment:35 Changed 4 years ago by vipsoft (robocoder)

  • Milestone changed from Features requests - after Piwik 1.0 to 0 - Piwik 0.6.4

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.

comment:36 Changed 4 years ago by matt (mattab)

Committed patch in [2392] - Thanks JulienM!

comment:37 Changed 4 years ago by matt (mattab)

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

comment:38 Changed 4 years ago by matt (mattab)

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.

comment:39 Changed 4 years ago by matt (mattab)

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?

comment:40 Changed 4 years ago by matt (mattab)

(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

comment:41 Changed 4 years ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from new to closed

Great job Julien!!

comment:42 Changed 4 years ago by matt (mattab)

(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.

comment:43 Changed 4 years ago by JulienM (JulienMoumne)

(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

comment:44 Changed 3 years ago by vipsoft (robocoder)

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

comment:45 Changed 3 years ago by Camille23HANEY

comment:46 Changed 13 months ago by 1

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

Note: See TracTickets for help on using tickets.