Add caching and streamline code to improve performance of skinr core module.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moonray’s picture

Status: Active » Needs review
FileSize
3.1 KB

Attached patch improves performance on all but the first page load to the same load speed as not having any skins configurations on a page.

Some testing results on 500 page requests:

  • Skinr module disabled: 185ms ± 5.8ms
  • Skinr + Skinr Context without patch, with 17 skin configs: 229ms ± 2.5ms
  • Skinr + Skinr Context without patch, with 0 skin configs: 223ms ± 5.3ms
  • Skinr + Skinr Context with patch applied, with 17 skin configs: 217ms ± 4.1ms
  • Skinr without patch, with 17 skin configs: 223ms ± 4.9ms
  • Skinr without patch, with 0 skin configs: 223ms ± 5.9ms
  • Skinr with patch applied, with 17 skin configs: 216ms ± 5.0ms

The results seem to indicate the following:

  • Skinr Context adds a relatively decent amount of overhead vs only Skinr core
  • The patch eliminates that overhead from Skinr Context (after the initial caching occurs)
  • The patch improves Skinr core performance slightly

I'd like to test this with a high number of skin configs to see if more skins means incrementally slower load times (which I suspect), and if the patch indeed reduces those increased load times across the board.

Some additional eyes on this patch would be appreciated.

Status: Needs review » Needs work

The last submitted patch, skinr-performance-2007416-1.patch, failed testing.

moonray’s picture

This patch is incompatible with skinr_context, which is why it failed. Needs work.

moonray’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.25 KB

With 1000 skins, this patch offers the following performance enhancements:

Pre-path:
616ms +/- 6.8ms

Post-patch:
371ms +/- 7.7ms

That's a significant increase. The more skins are displayed on the page, the better the enhancements this patch adds.
A BIG caveat, if skinr_context module is enabled (or any module implementing skinr_preprocess_alter()), this patch's caches are skipped.

Status: Needs review » Needs work

The last submitted patch, 4: skinr-performance-2007416-4.patch, failed testing.

moonray’s picture

Status: Needs work » Needs review
FileSize
5.23 KB

Removed stray dpm(). Retesting.

  • Commit 19d1d68 on 7.x-2.x by moonray:
    Issue #2007416 by moonray: Improved skinr_preprocess() performance.
    
moonray’s picture

Status: Needs review » Active

Leaving this open for follow-up patches. This can always be tweaked further.

  • moonray committed 19d1d68 on 8.x-2.x
    Issue #2007416 by moonray: Improved skinr_preprocess() performance.