1352264 - Preload all templates

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, enhancement, P2)

Description

[Dylan Hardison [:dylan] (he/him)]

The template provider is loaded in each worker, so each worker ends up stat'ing the same files. Eventually each worker has loaded every template, only to repeat this again. There are really not that many templates.

With this and the .htaccess thing, a running bugzilla only runs stat() about once per request.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1352264_draft_1.patch

This change is drastic and I'm sure this patch isn't in a reviewable state. However.. this is fairly insane: I can avoid... 3590 calls to stat per request.

There was some weird (very broken) behavior until I made 'constants' a variable. Note that it shouldn't have worked before any way, as many things in constants are not knowable at compile time.

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1352264_1.patch

This work in progress is tremendous. It saves time on stat(), File::Spec->catfile (because we don't need to do path searching) and memory.
Why memory? Because fork() and copy-on-write memory -- most of the template memory is in the parent HTTP process. So that process is a bit larger, and each of the children is dramatically smaller (at least when you look at the unshared size)

Comment 3

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This sounds like this is already being worked on, so marking P1.

Comment 4

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Is this the same as bug 1351374?

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Nope! Although with this the wins from 1351374 are much smaller.

Basically this bug means templates are only parsed once at startup,
and no file accesses are made. Bug 1351374 is about the Template object.

Comment 6

[:glob ✱]

Comment on attachment 8853623 [details] [diff] [review]
1352264_1.patch

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

::: Bugzilla/Template/Provider.pm
@@ +5,5 @@
> +# This Source Code Form is "Incompatible With Secondary Licenses", as
> +# defined by the Mozilla Public License, v. 2.0.
> +
> +# This exists to implement the template-before_process hook.
> +package Bugzilla::Template::Provider;

for clarity please rename this to ModPerlProvider

@@ +26,5 @@
> +    $self->{_BZ_CACHE} = $cache;
> +    $self->{_BZ_SEARCH} = $search;
> +
> +    foreach my $template_dir (@$path) {
> +        $template_dir = Cwd::realpath($template_dir);

add Cwd to 'use' list at top of file

@@ +29,5 @@
> +    foreach my $template_dir (@$path) {
> +        $template_dir = Cwd::realpath($template_dir);
> +        my $wanted = sub {
> +            package File::Find;
> +            use File::Slurper qw(read_text);

File::Slurper isn't listed as a required module.

use File::Slurp instead.  moving to File::Slurper makes a lot of sense, but that should happen in another bug.

@@ +31,5 @@
> +        my $wanted = sub {
> +            package File::Find;
> +            use File::Slurper qw(read_text);
> +            use Taint::Util qw(untaint);
> +            our ( $name, $dir );

i'd prefer to see this use a more traditional way of consuming File::Find.
ie. don't inject this code into the File::Find package, use $File::Find::name and $File::Find::dir to access vars.

@@ +37,5 @@
> +                my $key = $name;
> +                $key =~ s/^\Q$template_dir\///;
> +                unless ($search->{$key}) {
> +                    $search->{$key} = $name;
> +                }

$search->{$key} //= $name

@@ +56,5 @@
> +
> +sub fetch {
> +    my ($self, $name, $prefix) = @_;
> +    my $file;
> +    if (File::Spec->file_name_is_absolute($name)) {

add File::Spec to 'use' list at top of file

@@ +82,5 @@
> +sub _bz_compile {
> +    my ($self, $data) = @_;
> +
> +    my $parser = $self->{PARSER} ||= Template::Config->parser( $self->{PARAMS} )
> +        || return ( Template::Config->error(), Template::Constants::STATUS_ERROR );

Template::Config and Template::Constances should already be loaded, but since you're referencing them they should be added to the 'use' list.

@@ +95,5 @@
> +            'modtime' => $data->{time},
> +            %{ $parsedoc->{METADATA} },
> +        };
> +
> +        return Template::Document->new($parsedoc);

and Template::Document too.

::: extensions/BMO/template/en/default/bug/create/create-poweredby.html.tmpl
@@ +17,4 @@
>    # Rights Reserved.                                                          
>    #                                                                           
>    # Contributor(s): Gervase Markham <[e-mail]>                       
> +  #                 Ville Skytt <[e-mail]>

this change is unrelated.

::: mod_perl.pl
@@ +79,4 @@
>  
>  Bugzilla->preload_features();
>  
> +Bugzilla->template;

add a comment here, something like "trigger template preloading"

Comment 7

[Dylan Hardison [:dylan] (he/him)]

> ::: extensions/BMO/template/en/default/bug/create/create-poweredby.html.tmpl
> @@ +17,4 @@
> >    # Rights Reserved.                                                          
> >    #                                                                           
> >    # Contributor(s): Gervase Markham <[e-mail]>                       
> > +  #                 Ville Skytt <[e-mail]>
> 
> this change is unrelated.

Unrelated but required as it is not valid utf8. I will commit a small 'nit' patch before this one to remove the offending bytes.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1352264_2.patch

Changes addressed in patch, also here are some benchmarks for fun:

Before:
Concurrency Level:      4
Time taken for tests:   39.540 seconds
Complete requests:      157
Failed requests:        0
Total transferred:      22507834 bytes
HTML transferred:       22339530 bytes
Requests per second:    3.97 [#/sec] (mean)
Time per request:       1007.376 [ms] (mean)
Time per request:       251.844 [ms] (mean, across all concurrent requests)
Transfer rate:          555.91 [Kbytes/sec] received

Concurrency Level:      4
Time taken for tests:   34.301 seconds
Complete requests:      157
Failed requests:        0
Total transferred:      22505950 bytes
HTML transferred:       22337646 bytes
Requests per second:    4.58 [#/sec] (mean)
Time per request:       873.918 [ms] (mean)
Time per request:       218.480 [ms] (mean, across all concurrent requests)
Transfer rate:          640.75 [Kbytes/sec] received

Comment 9

[:glob ✱]

Comment on attachment 8856707 [details] [diff] [review]
1352264_2.patch

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

r=glob

Comment 10

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   effcc51..c82963a  master -> master

Comment 11

[Dylan Hardison [:dylan] (he/him)]

This regresses rendering comment previews.

Comment 12

[Dylan Hardison [:dylan] (he/him)]

To github.com:mozilla-bteam/bmo.git
   b1aece569..48cf53e5b  master -> master