I've looked a bit at the bundles that get generated. Generally, think there are too many that include jquery.js (or jquery.min.js).

The existing bundler module allows one to restrict the number of bundles that are put into one page.

What we want instead is a limit on the maximal file size in addition to that. That is, all files that are over a certain size limit (e.g. 100k) will never be added to a bundle but always will constitute a bundle of their own, possibly simply a link to the file. This would potentially increase the limit of the bundles per page but would make sure that big files are only downloaded once by a partiular user for the whole site.

To do that we need to:

1) change the main advagg module to also record file sizes, not only md5 hashes etc.

2) rewrite the bundler module (or perhapes create a custom module) that copies most of the existing module but has the altered functionality that I described.

Opinions?

Files: 
CommentFileSizeAuthor
#7 advagg-1196434-7.patch1.11 KBmikeytown2

Comments

crea’s picture

I think best option would be to implement #1140624: Create GUI for full control over bundles used. Because of potential complexity of involved algorithms, It's much better when human manages this task.
But this leaves the question of the entry level for users of this module (i.e. site admins) because to make bundles one must understand the whole process and its purpose. If we want to support newbie users, then the module needs automatic mode anyway.

mikeytown2’s picture

In terms of jQuery; I use the AdvAgg CDN JS module and get jQuery from Google's CDN. Having a max aggregate file size could be another module; it would turn off aggregation for that file if it's over a certain size. It would use hook_advagg_js_pre_alter & hook_advagg_css_pre_alter to turn off preprocessing for that file. Store the file size in the cache via advagg_get_file_data()/advagg_set_file_data(); which needs locking now that I think about it #1197280: race condition

In terms of the bundler I'm thinking about dropping in a drupal_alter in advagg_bundler_analysis on $analysis after db_fetch_array is done. The advagg_bundler_analysis() function is simple, and it's pretty good for how dumb it is. It could take into account how often a file changes and having the file size as a row in the "advagg_files" would be a good idea to try to make a better bundle. Adding a GUI on top of it is the ultimate in terms of tweaking, but the issue that can arise from allowing that is the order of JS execution and the order of CSS because it cascades. I've ran into issues with trying to get better JS selection, see advagg_find_existing_bundle(); I no longer use it due to issues I was having in the browser. If we had a dependency tree for the JS files that would be helpful, but that not possible for D6 or D7.

In terms of the bundler's GUI what do you have in mind & what operations would you allow?

crea’s picture

On GUI stuff: I recommend to start with simple variable-based setup. Making full featured GUI could take a lot of time. Meanwhile its much better to have something like this working for advanced users who don't use a GUI for such tasks. I would go even further and suppose most users who care about performance are comfortable without GUI.
I would allow to manually define bundles, and to assign files between them. Nothing really special at this point.

mikeytown2’s picture

mikeytown2’s picture

I could create some logic that would figure out the dependency tree of the css & js files. I record the order so this could be done... just a thought. With that one might be able to create a master js file.

killes@www.drop.org’s picture

I am not much interested in having a GUI and would prefer to have everything as automated as possible.

I think the proposed drupal_alter in the analysis function could be good enough for me. I'll see how far I get. I'll also work on recording the file size.

mikeytown2’s picture

StatusFileSize
new1.11 KB

I've committed this patch to add in an alter hook. The filesize patch has already been committed. Leaving this issue open.

mikeytown2’s picture

In terms of a master set here is some of the code I've been playing around with. It's sorted so the largest bundles appear in the output.

<?php
require_once './includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

$results = db_query("
  SELECT *
  FROM {advagg_bundles} AS advagg_bundles
  INNER JOIN {advagg_files} AS advagg_files USING (filename_md5)
  INNER JOIN (
    SELECT
      COUNT(*) AS file_count,
      bundle_md5
    FROM {advagg_bundles} AS advagg_bundles
    GROUP BY bundle_md5
  ) AS advagg_bundles_count USING (bundle_md5)
  WHERE root = 1
  AND filetype = 'js'
  ORDER BY
    file_count ASC,
    porder ASC
"
);

$data = array();
$raw_data = array();
while (
$row = db_fetch_array($results)) {
 
$data[$row['filename']][$row['porder']] = array(
   
'filename'      => $row['filename'],
   
'porder'        => $row['porder'],
   
'bundle_md5'    => $row['bundle_md5'],
   
'filename_md5'  => $row['filename_md5'],
   
'file_count'    => $row['file_count'],
  );
 
$raw_data[$row['bundle_md5']][$row['porder']] = $row;
}

$master_set = array();
foreach (
$data as $filename => $values) {
 
// Get the last key value from the $values array
 
end($values);

 
// Save to the master set. order and info
 
$master_set[key($values)][] = current($values);
}

// Sort the array by the order
ksort($master_set);

echo
str_replace('    ', '&nbsp;&nbsp;&nbsp;&nbsp;', nl2br(htmlentities(print_r($master_set, TRUE))));
?>

Output looks like this.

Array
(
    [0] => Array
        (
            [0] => Array
                (
                    [filename] => sites/all/modules/wysiwyg/wysiwyg.js
                    [porder] => 0
                    [bundle_md5] => 057c9391e7eebe6ee2528be1a1abf379
                    [filename_md5] => 057c9391e7eebe6ee2528be1a1abf379
                    [file_count] => 1
                )

            [1] => Array
                (
                    [filename] => sites/all/modules/labjs/drupal.modified.js
                    [porder] => 0
                    [bundle_md5] => e3e4e374cd27481ad0a04c6a94b8fa54
                    [filename_md5] => e8d4e7153fa7bc38a0ae7ad37d6d3d23
                    [file_count] => 25
                )

            [2] => Array
                (
                    [filename] => misc/drupal.js
                    [porder] => 0
                    [bundle_md5] => 71b6b3aa0ec3b095c9df5b8a451f0e00
                    [filename_md5] => 52ae9b20d07a214cf2b5eb3bbd675899
                    [file_count] => 31
                )

        )

    [1] => Array
        (
            [0] => Array
                (
                    [filename] => sites/all/modules/wysiwyg/wysiwyg.init.js
                    [porder] => 1
                    [bundle_md5] => 71b6b3aa0ec3b095c9df5b8a451f0e00
                    [filename_md5] => e92fe532562ef973a49397c73e8488fe
                    [file_count] => 31
                )

        )

    [2] => Array
        (
            [0] => Array
                (
                    [filename] => sites/all/modules/jquery_update/replace/tabledrag.js
                    [porder] => 2
                    [bundle_md5] => 71b6b3aa0ec3b095c9df5b8a451f0e00
                    [filename_md5] => 6aff4e01ed5a1358ed03c81312b9b436
                    [file_count] => 31
                )

        )

    [3] => Array
        (
            [0] => Array
                (
                    [filename] => sites/all/modules/admin_menu/admin_menu.js
                    [porder] => 3
                    [bundle_md5] => 71b6b3aa0ec3b095c9df5b8a451f0e00
                    [filename_md5] => 437033d6f659c6a1a62c71e8bd4214e2
                    [file_count] => 31
                )

        )

    [4] => Array
        (
            [0] => Array
                (
                    [filename] => sites/all/modules/domain_tweets/jquery.timeago.min.js
                    [porder] => 4
                    [bundle_md5] => 71b6b3aa0ec3b095c9df5b8a451f0e00
                    [filename_md5] => 0dc1450974648a310b26653371f9f487
                    [file_count] => 31
                )

        )

    [5] => Array
        (
            [0] => Array
                (
                    [filename] => sites/all/modules/domain_tweets/jquery.jsonp.min.js
                    [porder] => 5
                    [bundle_md5] => 71b6b3aa0ec3b095c9df5b8a451f0e00
                    [filename_md5] => d4f55cb7b3b57b49a99c1147e576d8f8
                    [file_count] => 31
                )

        )

...

    [14] => Array
        (
            [0] => Array
                (
                    [filename] => sites/all/modules/views_javascript_random/views_javascript_random.js
                    [porder] => 14
                    [bundle_md5] => 71b6b3aa0ec3b095c9df5b8a451f0e00
                    [filename_md5] => f4481865eee1dd1acd14193e18570a9e
                    [file_count] => 31
                )

            [1] => Array
                (
                    [filename] => sites/all/modules/panels/js/layout.js
                    [porder] => 14
                    [bundle_md5] => 0246ef2a4bc53cf822450f2f9d4679ee
                    [filename_md5] => 68d9fb142bc55219843234ca33760610
                    [file_count] => 21
                )

            [2] => Array
                (
                    [filename] => sites/all/modules/ctools/js/dropdown.js
                    [porder] => 14
                    [bundle_md5] => e3e4e374cd27481ad0a04c6a94b8fa54
                    [filename_md5] => 094f688c722a163aacfa44752241a303
                    [file_count] => 25
                )

        )

...

    [29] => Array
        (
            [0] => Array
                (
                    [filename] => sites/all/modules/jquery_update/replace/jquery.form.js
                    [porder] => 29
                    [bundle_md5] => 71b6b3aa0ec3b095c9df5b8a451f0e00
                    [filename_md5] => 55fedf461696441f3cfa2fcb998e6c08
                    [file_count] => 31
                )

        )

    [30] => Array
        (
            [0] => Array
                (
                    [filename] => sites/all/modules/jquery_update/replace/ahah.js
                    [porder] => 30
                    [bundle_md5] => 71b6b3aa0ec3b095c9df5b8a451f0e00
                    [filename_md5] => 3de29571ae14527969763f109c6c2f3a
                    [file_count] => 31
                )

        )

)

So in this case there are 3 files trying to get into the #0 position. One bundle is for the footer and the other 2 depend on if labjs is enabled. For #14 we have 3 different bundles; looks like the two odd ones have to do with the admin section of panels.

mikeytown2’s picture

Thinking about this and I think if I move advagg_find_existing_bundle from where it currently is to before hook_advagg_filenames_alter gets called that might do what a lot of us are looking for. Ideally it would grab the largest bundle that has all the files in it that we are looking for; but then it doesn't handle js files getting removed. Sounds like I need a valid flag on the bundle; if it includes a file that no longer exists then don't use that bundle. Which still doesn't work because most people just remove the drupal_add_js reference & not the file.

Example: If some one enables the External Links module and after having it on for a month they decide to disable it. External Links adds its JS in hook_init so it will be on every page. If going for one big JS file, External Links JS will still be loaded because it is added to the bigger bundle; bigger is usually better, but here it is wrong. In this case you would have to manually remove extlink.js from the bundle. You could scan for disabled modules and mark js files in the disabled module dir as not valid, but that doesn't account for JS added from libraries. I think for Drupal 8, if we record what module is calling drupal_add_js that would make the creation of a master JS file a lot easier.

All of this explains why I won't be adding in the ability to include JS files that are part of a bigger bundle; it breaks Drupal in a not so obvious way. advagg does not currently have this issue (doesn't allow for it), but I hope this explains why I won't be writing the module that does this; at least not for D6 or D7. Now if we had a GUI then we could support some fairly advanced logic, but when you uninstall a module that's one more thing that needs to be checked; and that sounds like a support nightmare, OR I could truncate the advagg database when a module gets disabled but then you would lose 404 protection.

Using advagg_find_existing_bundle (in strict mode) near the top of advagg_get_filename might be an option I would consider; it would be a setting because if a module decides to change the order of js files around it won't pick it up.

Every time I come up with a way to make this better I find an edge case that makes it not work. This might explain why bundlecache has never really caught on. Coming up with better logic in advagg_bundler_analysis does make perfect sense, but trying anything more extreme might cause some unforeseen consequences (sorry about this last link).

mikeytown2’s picture

Status:Active» Closed (won't fix)

Do to all the edge cases that might come up, I'm going to mark this as won't fix. A 3rd party module can easily accomplish this goal.