The template toolkit can pre-compile templates into Perl code, which is a performance win. Enabling the feature is trivial. It is just a configuration parameter: http://www.template-toolkit.org/docs/aqua/Manual/Config.html#Caching_and_Compiling_Options my $template = Template->new({ COMPILE_EXT => '.atmlc', });

Then why didn't we do it?

And why wasn't it done in the first place?

> Then why didn't we do it? We didn't know about it. > And why wasn't it done in the first place? See above.

I take it that it notices when the template has changed? Gerv

We are currently trying to wrap up Bugzilla 2.16. We are now close enough to release time that anything that wasn't already ranked at P1 isn't going to make the cut. Thus this is being retargetted at 2.18. If you strongly disagree with this retargetting, please comment, however, be aware that we only have about 2 weeks left to review and test anything at this point, and we intend to devote this time to the remaining bugs that were designated as release blockers.

*** Bug 120770 has been marked as a duplicate of this bug. ***

I'm going to move this back to 2.16. I enabled template precompilation, applied my patch for bug 100094 to make all tempaltes use the global defn, and then did: time for i in `seq 1 50`; do wget -O - http://localhost/bugzilla-misc/query.cgi > /dev/null; done on an otherwise unloaded PIII-500 with 256 megs of ram With template compilation - 1 minute 6 sec without - 1 minute 59 seconds For index.cgi: With - 59 seconds Without - 1 minute 12 seconds There was a noticable pause in the wget output before the download started when templates were not precompiled. These numbers were repeatable to within a couple of seconds each way, and the with version was run once separately to allow the initial compile to occur Watching via top, it appeared that the apache process took more CPU time w/o compiled templates, but I'm not really sure about that - even with compiled templates apache was occasionally hitting 70% CPU. This was probably the 'start the perl interpreter' overhead. There is, as always a catch. Loading from precompiled templates is broken in TT2.06 - see http://tt2.org/pipermail/templates/2002-January/002361.html Its fixed in TT2.06d, though, and I applied that patch before running the above tests. The patch to enable template compilation is trivial (And in fact its on my patch to bug 100094, by accident). I'm using COMPILE_DIR => data/templates rather than cluttering up the template directory. This handles the custom vs default INCLUDE_PATH for us - we don't have to worry about that. Doing precompilation in checksetup.pl is a tiny bit more effort, and is probably not worth it anyway, since it is automatically created if it doesn't exist Moving back to 2.16 for reconsideration. A 44% speedup for complex templates is quite a bit, but we are sort of in a freeze. There may even be a very small ammount of overhead if this is enabled with 2.06, because the regenerated template is written to disk every time due to the TT bug. I couldn't measure that difference, though, but that may be because the file was in my disk cache. Comments?

This is the tt2 patch from the message I mentioned, unwrapped so that it applies.

This is the tt2 patch from the message I mentioned, unwrapped so that it applies.

Comment on attachment 68115 [details] TT patch stupid useless broken web proxies

This one is important to get into 2.16 but useless with TT 2.06, so we should either remove the dependency on that version and get people to stick with TT 2.04 or wait and see if TT 2.07 gets released and require that version. Third option: pref it with a parameter and warn users about the bug.

How about some stats on disk space used? If significant, we might want to make this an editparams option.

If I'm correct, we only require TT 2.06 because of the "uri" filter. Now that 100094 has landed, we could easily implement our own version of that and bump our TT requirement back down to 2.04. This is an unpalatable option, but one we should consider if necessary.

100094 hasn't quite landed, yet. However: 2.04 may have the same bug (I'd have to test). and people may have 2.06 anyway. since testing for the existance of the bug is hard, I'll make it a param, and won't precompile. After 2.07 is released, we can require that, and then I'll add some precompile stuff into checksetup.pl

Template Toolkit version numbers do go up a lot faster than Bugzilla version numbers, so it's entirely possible that 2.07 will be released in the next week. If it isn't, I guess we'll have to figure out what to do. Perhaps continue to require 2.06, but says if you have 2.07 or later, you can get a performance boost by turning on template compilation...

If the performance hit in the new version of TT's "html" filter (in TT 2.06d) does not get corrected, we may want to override that filter with our own simpler version, in which case we can drop our required version back down to 2.04.

No, 2.06 was for the uri filter, not for HTML. The patch is trivial - add the one line to globals.pl and change checksetup to remove the directory. I'll wait a bit - lets see what happens with the TT release schedule.

>No, 2.06 was for the uri filter, not for HTML. Err, right, override that one too. :-)

This is included in your patch on bug 100094 according to comment 7... is this still correct? If not, remove the "blocker will fix" from the whiteboard.

No, we decided to consider this as a separate issue.

Is TT 2.07 anywhere near light of day yet? If not, we need to file a new bug against 2.16 for the workaround and push this out until 2.07 is out...

Lets wait until the weekend. If theres no sign of another TT release by then, then I'll turn it on, and document the fact that it won't do anything useful without applying the patch here.

I did some tests on index.cgi, since its the template with the least backend processing. using wget to load it 50 times > /dev/null: - current source: 1 min 11 sec - TT2.06 with template compilation: 1 min 14 sec - TT2.06patched with template compilation = 0 min 56 sec So, do we want to turn it on by default, and document the patch? Its a 4% slowdown vs a 20% speedup.

::sigh:: I hate patching other peoples' products to make ours work. But that certainly looks like the most attractive option. Is this something that's going to be included in TT 2.07? If so, lets go ahead and document the patch, and let people know that TT 2.07 will probably remove the necessity for the patch.

The biggest pain with this is that we need to change checksetup so that we can delete compiled templates. The problem with doing that is that we have to get the permissions right so that the webserver created files are still deletable by the user, even when apache is run under a different username. Just setting the special bits on data/template doesn't help with subdirs created by the server.

I'll also point out that the 20% speedup is a bit artifical, because 'real' pages will actually involve the db. Its still likely to be a reasonable ammount, though.

Also, if 2.07 is released before we do (its in RC2 now, basically), then I'll pull the stuff from the relnotes, and require 2.07 instead.

Adding dependency to bug 135707 since the patch will change slightly after all the *.atml files have been renamed.

I don't understand permissions well enough to review this; they make my head spin :-| Gerv

Comment on attachment 72209 [details] [diff] [review] v1 Index: globals.pl >+# XXX - it is vital that these settings be kept up to date in the template test, >+# and checksetup.pl Nit: This is not as clear as it could be. It would be useful to add "If you make any configuration changes here, make sure to make them in NAME_OF_TEMPLATE_TEST and checksetup.pl." to this comment. Index: checksetup.pl >+ return if ($_ eq "."); Nit: unnecessary parentheses. >+ # Precompile stuff. This speeds up initial access (so the template isn't compiled multiple >+ # times simulatniously by different servers), and helps to get the permissions right. Nit: simultaneously. >+ sub compile { >+ return if (-d $_); >+ return if ($_ !~ /\.(atml|tmpl)$/); This will be just tmpl in a day or two. >+ s!template/default/!!; This will be template/en/default in a day or two. >+ find({wanted => \&compile, no_chdir => 1}, "template/default"); Do we not want to pre-compile custom templates? >+ open FILE, '>data/template/.lastRebuild'; close FILE; >+ utime $lastTemplateParamChange, $lastTemplateParamChange, ('data/template/.lastRebuild'); Why is it necessary to open and close the file first?

We do compile custom templates, because of the INCLUDE_PATH. We iterate over just the default stuff, on the basis that all the 'root' templates have names in default. If it is a custom one, then TT will pick that up instead, and if that includes one we haven't seen, TT will compile it anyway, as processsing each temaplte is recursive. The file has to exist to set the utime, this is the equivalent of system ("touch", "foo"); Also, is there a better way to remove an entire directory tree?

TT 2.07 is out. And installed on Landfill, both in Perl 5.6.1 and Perl 5.005.

Is this ok for TT 2.07 for use with bugzilla? Failed Test Status Wstat Total Fail Failed List of failed ------------------------------------------------------------------------------- t/cgi.t 13 3 23.08% 9, 11, 13 t/context.t 54 2 3.70% 50-51 t/date.t 33 1 3.03% 25 t/foreach.t 89 8 8.99% 35, 37, 39, 41, 45, 55, 57, 59 t/include.t 47 1 2.13% 27 t/view.t 110 1 0.91% 78 t/vmeth.t 87 2 2.30% 55, 57 16 tests skipped. Failed 7/85 test scripts, 91.76% okay. 18/2254 subtests failed, 99.20% okay.

what's different about your system? All the tests passed on Landfill (both in Perl 5.00503 and Perl 5.6.1) when I installed it there via CPAN.

Comment on attachment 79698 [details] [diff] [review] patch v2: a version of bbaetz' patch that applies to the tip >Index: checksetup.pl >+if ($my_webservergroup && $< != 0) { # zach: if not root, yell at them, bug 87398 Prior experience tells me that && has precedence problems with comparisons. Probably need to add parens here. if ($my_webservergroup && ($< != 0)) { # ...

OK, we now require TT 2.07. I've made the suggested changes, and also fixed a bug where we'd get a warning if data/template didn't exist. Theres one problem, though - if you upgrade TT versions, then the toolkit won't recompile stuff. That could be considered a TT bug, mind you, and I think I will report it to the list. Diffing the 2.06 vs 2.07 stuff, 2.07 just has fewer redundant brackets, but there could be parsing/code generation bugs fixed in the future which would make a difference.

> what's different about your system? Don't know. Anyway, TT 2.07 works for me as-is. And the test results are still better (wrt. subtests) than the ones for 2.06 which were: Failed 4/82 test scripts, 95.12% okay. 46/2004 subtests failed, 97.70% okay.

Did you remember not to run teh database tests if you have a password for teh user you're installing as? Anyway, moving to blokcer, as this MUST be done for release.

Comment on attachment 79740 [details] [diff] [review] v2 >+ unless (-d 'data/template' && -e 'data/template/.lastRebuild' && Nit: we tend to go for lower-case, hyphen-separated filenames. >+ # Colon-separated list of directories containing templates. >+ INCLUDE_PATH => "template/custom:template/default" , This needs fixing to include the "en" directories. >+ $suidperm ||= 0; >+ my $dirperm = $execperm + $suidperm; You create this variable and don't seem to use it. >+# XXX - If you make any configuration changes here, make sure to make them >+# in t/004.template.t and checksetup.pl. You may also need to change the >+# date settings were last changed - see the comments in checksetup.pl for >+# details Don't use XXX to mark this - XXX implies broken or unfinished code. Use "Important:" or something like that. >+ COMPILE_DIR => "data", data/template? Gerv

>(From update of attachment 79740 [details] [diff] [review]) >>+ unless (-d 'data/template' && -e 'data/template/.lastRebuild' && >Nit: we tend to go for lower-case, hyphen-separated filenames. >>+ # Colon-separated list of directories containing templates. >>+ INCLUDE_PATH => "template/custom:template/default" , >This needs fixing to include the "en" directories. only once we change to using those. Its a trivial fix - don't needswork for that. >>+ $suidperm ||= 0; >>+ my $dirperm = $execperm + $suidperm; >You create this variable and don't seem to use it. It is used. >>+# XXX - If you make any configuration changes here, make sure to make them >>+# in t/004.template.t and checksetup.pl. You may also need to change the >>+# date settings were last changed - see the comments in checksetup.pl for >>+# details Don't use XXX to mark this - XXX implies broken or unfinished code. Use "Important:" or something like that. OK. >>+ COMPILE_DIR => "data", >data/template? No, INCLUDE_PATH is placed at the end. So we end up with data/template/custom and data/template/default (and data/template/en/{custom,default} with your changes) Yes, this does imply that is has to be data/template, not data/compiled-templates (which is the name I wanted originally). It could have been data/compiled-templates/template/en/default, but....

afranke: From the TT list, if you don't have Tie::DBI installed, then the tests won't be wrong, but the cout will be incorrect, so some of the dbi stuff will be listed as failing. That may be what you're seeing.

bbaetz: yes, it looks like I don't have Tie::DBI.

>Anyway, moving to blokcer, as this MUST be done for release. I don't see any reason why this bug must be fixed before release. Template pre-compilation is useful for sites that need the additional performance boost, but it isn't necessary, even for those sites, and not having it has very little effect on the majority of Bugzilla installations the majority of the time. Dropping severity to reflect reality. This doesn't mean I'm not willing to review this before release (having it certainly makes my job as administrator of a large Bugzilla installation for which performance does matter easier, since otherwise I would do this on that installation anyway), just that it doesn't qualify for blocker status by any reasonable criteria I have seen or can imagine.

> >You create this variable and don't seem to use it. > > It is used. I did look quite carefully - I don't think it's used in sub fixPerms(), where that declaration is. But I'm happy to be enlightened. :-) > No, INCLUDE_PATH is placed at the end. A comment to that effect might be good. Gerv

*** Bug 139873 has been marked as a duplicate of this bug. ***

OK, take 3. I talked on IRC with people over the perms issue, and came up with this. See the relnotes for what we do. Basically, if you don't have a webservergroup, then you need to rerun checksetup every time you cahnge a template. If you do, then it will work, except that: If you add a directory (eg something in custom) AND don't rerun checksetup before viewing it in teh browser AND you don't run checksetup as root, then you may get permission errors, because you won't own the directory in question. How does that sound? Oh, and I also removed the .ttc extention, for no particular reason.

Comment on attachment 80933 [details] [diff] [review] v3 The release note is a bit messy, but otherwise this is fine and works. r=myk

> See the relnotes for what we do. Basically, if you don't have a webservergroup, > then you need to rerun checksetup every time you cahnge a template. That kind of sucks. I currently change templates all the time, and I don't normally run with a webservergroup (because you need to add it in every time you re-check-out Bugzilla, which I do regularly.) I also don't run checksetup as root, because that's a pain as well. Currently, everything Just Works for me. (I've added myself to the apache group, and apache to mine. This is fine; I'm behind NAT.) I'd be very sad if things stopped Just Working. Gerv

You also need to set the db password every time you check out bugzilla, so... All you actually need to ensure is that the webserver has write access to the appropriate directories (data/template/*). If you run with suexec (yes, I've read the arguments in bugzilla against it, and I don't buy it) then that will be you, and there will be no problem. Or I can do what Dave suggested, and just not use precompiled templates if there is no webserver group.

gerv is going to play with this, and see if it breaks. I don't want to change this to cater for gerv's somewhat unique setup. OTOH, since I've discovered that if you don't have a webservergroup you have no security at all anyway (See bug 139930), do we just want to let these template directories be world writable?

Comment on attachment 80933 [details] [diff] [review] v3 Change the release note to: This release of Bugzilla uses the Template Toolkit. For speed, compiled templates are cached. If you modify the templates in order to customise the look and feel of your Bugzilla instalation, the toolkit will normally detect the changes, and recompile the changed templates. However, if you do not set a webservergroup in the localconfig file, (a generally unwise thing on a production installation of Bugzilla) the template directory would have to be world-writable for automatic recompilation to happen. Doing that would be a security risk. So, if you modify templates locally and do not have a webservergroup set, you will have to rerun checksetup.pl to recompile the templates manually. If you do not do this, the changes you make will not appear. Adding new directories anywhere inside the template directory may cause permission errors. If you see these, rerun checksetup.pl as root. (bug 79740) This works for me, and really speeds up page changes. r=gerv. Let's rock :-) Gerv

Checked in, with the last paragraph in the relnotes as: Adding new directories anywhere inside the template directory may cause permission errors. If you see these, rerun checksetup.pl as root. If you do not have root access, or cannot get someone who does to do this for you, you can rename the data/template directory to data/template.old (or any other name bugzilla doesn't use). Then rerun checksetup.pl to regenerate the compiled templates. Note that we may revisit this if we get complaints, and just make data/template world writable. The entire data directory is, so theres nothing stopping someone else from doing what I describe above.