I've been profiling a site where the sites/*/files directory is on glusterfs. On this the file_exists() for the google analytics javascript can be 20ms, while I haven't profiled it I think it'd also be an issue on sites using NFS as well.

I just posted a module http://drupal.org/project/agrcache that registers menu callbacks for css/js aggregates so that those paths are always guaranteed to exist, meaning they don't need to be checked when rendering the links to the css/js itself. The same ought to work for google analytics caching as well.

Don't have a patch yet but will hopefully work on one soonish.

CommentFileSizeAuthor
#9 1048520_googleanalytics_2.patch3.37 KBcatch
#7 1048520_googleanalytics.patch3.37 KBcatch
#5 1048520_googleanalytics.patch2.95 KBcatch
#3 1048520_googleanalytics.patch3.49 KBcatch
#2 1048520_googleanalytics.patch3.49 KBcatch

Comments

hass’s picture

hass commented

Another easy solution would be, not using local file caching, too. :-)

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
Issue tags: +Performance, +i/o
StatusFileSize
new 1048520_googleanalytics.patch3.49 KB

Got your message this morning, but by the time I'd got the patch ready the release was already up.

Here's the patch though, tested locally and appears to work fine. You only get the file_exists() optimization if you're using clean urls, otherwise it falls back to the current method.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 1048520_googleanalytics.patch3.49 KB

minus a typo.

hass’s picture

hass commented 
+++ googleanalytics.module
@@ -66,7 +66,15 @@ function googleanalytics_menu() {
+  $items[$directory_path . "/googleanalytics/%"] = array(

We may should better make the filename a fixed name to allow other ga releated modules to re-use the folder if they like.

+++ googleanalytics.module
@@ -341,6 +364,45 @@ function googleanalytics_cron() {
+    if (!file_exists($file_destination)) {

Topic of this thread is "Removing file_exists()", but we still have one!?

+++ googleanalytics.module
@@ -341,6 +364,45 @@ function googleanalytics_cron() {
+    $content_type = 'application/javascript';
+    $headers = array();
+    $headers['Content-Type'] = $content_type;

I would move the 'application/javascript' down and use it directly as value for 'Content-Type'.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 1048520_googleanalytics.patch2.95 KB

OK here's a revised patch. I hardcoded the filename in hook_menu(), no real need to make that generic. Also made the style change for Content-Type

On the file_exists() - I'm removing the file_exists() from the page rendering only.

Currently every page request to a site with this enabled has to check the existence of the cached javascript before it links to the js, and regenerates inline it if not. By ensuring the file is always available - with the menu callback taking over when it doesn't exist, we skip that file_exists(), and this also means that when the file doesn't exist, it doesn't hold up the page rendering at all (beyond the js file itself taking a bit longer).

The cron, and the checks in _googleanalytics_cache() are still needed, but those are much less of an issue since they'll run much more infrequently.

Status: Needs review » Needs work

The last submitted patch, 1048520_googleanalytics.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 1048520_googleanalytics.patch3.37 KB

Checkout was out of date, here it is against HEAD.

hass’s picture

hass commented
Status: Needs review » Needs work

Found one typo "applicatoin".

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 1048520_googleanalytics_2.patch3.37 KB

Ouch. Fixed in this patch.

hass’s picture

hass commented

@catch: Have you been able to get the basics into D8 core?

hass’s picture

hass commented
Issue summary: View changes
hass’s picture

hass commented
Status: Needs review » Closed (won't fix)

Since agrcache has not really a high usage and you can just disable the local caching without any negative effect I recommend doing this as a workaround until core implements something intelligent with magic...