Currently, SiteCatalyst regenerates variable data on every page request, inside a foreach loop. This is horribly slow, as token_replace() is horribly slow. The values of course rarely if ever change between requests. That makes them very cacheable, which can save quite a bit of render time on every page request.

This is essentially the same problem, and solution, as #2468531: tokens regenerated on every request, very slow for DFP.

Comments

Crell’s picture

Status: Active » Needs review
StatusFileSize
new3.94 KB

And patch. It should look an awful lot like that the one in the linked issue. :-)

Crell’s picture

StatusFileSize
new4.38 KB

New version, fixing various bugs.

In my quick benchmarking (on a VM so take with a grain of salt), this caching shaves about 260 ms off of every request. That's quite substantial. That number will of course vary widely depending on the number of token-providing modules installed and their individual cost, and how the module is configured. But as some of those (eg, entity_token) are not known for speed, this does seem worthwhile.

Per the discussion in #2468531: tokens regenerated on every request, very slow the cache key generation is not quite fully reliable yet; we probably need to include entity_changed module as a new dependency. :-(

Edit: Updated time report; as I was off by a decimal. :-)

Crell’s picture

StatusFileSize
new5.38 KB
new3.36 KB

Here's a version with the dependency added. This should have a more reliable key. I also added clearing of the cache when changing configuration.

Status: Needs review » Needs work

The last submitted patch, 3: 2468647-caching-dependency.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
StatusFileSize
new5.47 KB
new1.08 KB

And without a stupid bug in the key generation. This passes for me locally; let's see if the bot agrees.

Status: Needs review » Needs work

The last submitted patch, 5: 2468647-caching.patch, failed testing.

Crell’s picture

That's fascinating. I don't get anything even remotely like that set of fails locally...

Crell’s picture

I just tried it on a completely fresh Drupal install, too. Works fine, no errors. Maybe it's related to the new dependency? Is that confusing testbot, perhaps?

Crell’s picture

StatusFileSize
new6.36 KB

Revised version. This now sets the cache item separately per path to avoid the one cache item becoming ginormous. It also adds a config variable to allow the cache to be persisted longer than the next system cron. (It's X seconds, plus the next system cron. X defaults to 0.)

Unfortunately I have no further time on this client project to work on it, and this is working for us locally. So unless it fails for us for some reason I have to bail out here. bleen, hopefully you can carry it on home.

Crell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: 2468647-caching.patch, failed testing.

  • bleen18 committed 9fb31c1 on 7.x-1.x authored by Crell
    Issue #2468647 by Crell: tokens regenerated on every request, very slow
    
dalin’s picture

Assigned: Crell » Unassigned
Status: Needs work » Fixed

Since this is committed, changing to "fixed".

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.