Closed Bug 778631 Opened 8 years ago Closed 8 years ago

use a persistent Template::Provider to avoid recompiling templates between page loads on mod_perl

Categories

(Bugzilla :: Bugzilla-General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: glob, Assigned: glob)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

Attached patch patch v1 (obsolete) — Splinter Review
the normal way for a template-toolkit app to run under mod_perl is to have a single instance of Template per process, and reuse that for each request (bug 343346).  after investigation this doesn't appear to be trivial.

a simple fix is to use a single Template::Provider per process.

Template::Provider is the object which finds and loads the templates from disk, compiles, and manages two caches of the compiled template -- one one disk (in data/template) and one in memory.

currently the memory cache keeps bugzilla from re-reading the compiled template from disk if a template is used more than once during the same request (eg bug/link.html.tmpl), however this memory cache is dropped at the end of each request.

this patch makes our provider per-request.
Attachment #647075 - Flags: review?(dkl)
When a compiled template is used across requests, are you sure we are not passing the same data to different users?
Blocks: 696235
Severity: normal → enhancement
(In reply to Frédéric Buclin from comment #1)
> When a compiled template is used across requests, are you sure we are not
> passing the same data to different users?

yes; the part which is being cached is the compiled perl form of the template, not the generated html content.
> this patch makes our provider per-request.

oops, that should be "per-process" not "per-request".
Comment on attachment 647075 [details] [diff] [review]
patch v1

Review of attachment 647075 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works as expected. My timings show some decrease in overall time for loading a series of heavy pages. r=dkl
Attachment #647075 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 4.4
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Template.pm
Committed revision 8332.

the release notes need to mention an increase in memory requirements, as well as updates to templates taking up to an hour to become active (both mod_perl only).
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: relnote
Resolution: --- → FIXED
this caused burning on tinderbox:

Removing existing compiled templates...
Precompiling templates...file error - account/auth/login-small.html.tmpl: not found

i've backed this out:

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Template.pm
Committed revision 8334.

and will investigate.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: approval+
Attached patch patch v2Splinter Review
bugzilla uses the template's INCLUDE_PATH to ensure the correct language file is loaded, so we need a separate provider object cached for each INCLUDE_PATH.

during template compilation, the INCLUDE_PATH is changed from using the default list to one which only contains the parent of the file being compiled.  patch v1 would have cached only the first directory, causing the failures.

both issues are resolved by using the INCLUDE_PATH as a key for multiple provider objects.  during compilation the provider objects created with the single-directory INCLUDE_PATHs are deleted after use.
Attachment #647075 - Attachment is obsolete: true
Attachment #649650 - Flags: review?(dkl)
Comment on attachment 649650 [details] [diff] [review]
patch v2

Seems to work well in my testing and still gives a small performance boost. Unfortunately for BMO at least it does create some large provider keys, i.e.:

/home/dkl/devel/htdocs/mod_perl/extensions/BMO/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/BrowserID/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/BzAPI/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/ComponentWatching/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/ContributorEngagement/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/FlagTypeComment/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/GuidedBugEntry/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/InlineHistory/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/LastResolved/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/REMO/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/RequestWhiner/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/SecureMail/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/SiteMapIndex/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/Splinter/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/TagNewUsers/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/TryAutoLand/template/en/default:/home/dkl/devel/htdocs/mod_perl/extensions/Voting/template/en/default:/home/dkl/devel/htdocs/mod_perl/template/en/default

It is depending on the number of extensions installed which BMO has quite a few so probably not that bad for normal installations. I don't remember there being a limit on hash key size either so that shouldn't be an issue.

r=dkl
Attachment #649650 - Flags: review?(dkl) → review+
Flags: approval?
(In reply to David Lawrence [:dkl] from comment #8)
> It is depending on the number of extensions installed which BMO has quite a
> few so probably not that bad for normal installations. I don't remember
> there being a limit on hash key size either so that shouldn't be an issue.

it won't take longer for longer keys to get matched: it will take infinitesimally longer to hash the key, but that's all.  (perl use the hash of the key to find the right chain, not the key value itself -- see http://www.perl.com/pub/2002/10/01/hashes.html for more info).
So if I understand correctly, bmo has something like 20 or 30 Template::Provider objects? How much memory does this consume?
Flags: approval? → approval+
(In reply to Frédéric Buclin from comment #10)
> So if I understand correctly, bmo has something like 20 or 30
> Template::Provider objects? How much memory does this consume?

no, we'll have one provider object as we only have one language installed.

there will be a persistent provider for each unique INCLUDE_PATH.  this path only changes when the language is changed (eg. "template/en/default:template/fr/default" vs "template/fr/default:template/en/default").  the number of installed extensions increases the length of the path, but not the number of different paths.

i wasn't able to see any noticeable differences when measuring memory usage on my development system (but it was very noisy so difficult to tell).
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Template.pm
Committed revision 8338.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Added to relnotes for 4.4.
Keywords: relnote
Keywords: perf
You need to log in before you can comment on or make changes to this bug.