_htmlpurifier_load() unconditionally calls variable_set() to set the current installed version of htmlpurifier. This is called by hook_filter_info(), and due to #993274: Remove double caching logic, hook_filter_info() is called on every request. Drupal 7 has locking for variable_initialize(), which means when the variable cache is cleared on every request, it's equivalent to the site only being able to serve one page per second (or more, depending on how long the lock is held for). Attached patch checks the current version to ensure we only set it if it's changed.

CommentFileSizeAuthor
htmlpurifier_variable_set.patch811 bytescatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

This patch looks very reasonable - it's certainly very bad practice to variable_set(0 without a check on the current value or in response to some admin action.

paul.lovvik’s picture

paul.lovvik CreditAttribution: paul.lovvik commented
Status: Needs review » Reviewed & tested by the community

The patch is clearly written and appears to be right on inspection. I just tested the patch on my local system to try to validate any performance improvement, and was able to do so. My system is a local dev environment, not a full HA architecture, but even so the performance improvement was measurable.

Before the patch, the variable_set call took 1.7% of the request time (measured when saving a node).
After the patch, variable_set was not called, so it reduced the time for the call to 0% of the request time.

In architectures in which the DB is not on the same hardware as the web server, I am certain this difference in performance would be much more pronounced.

ezyang’s picture

ezyang CreditAttribution: ezyang commented

Thanks for the patch. I am currently on vacation, so I can't promise a time when I'll put the patch in, but it looks quite reasonable.

ezyang’s picture

ezyang CreditAttribution: ezyang commented
Status: Reviewed & tested by the community » Fixed

Committed to 6.x and 7.x. Thanks for the fix.

Status: Fixed » Closed (fixed)

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

jcisio’s picture

jcisio CreditAttribution: jcisio commented

I'm having the same issue with this. This is a critical issue so I think it worth a official release (as there is no new release since more than a year).