Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dylan, Assigned: dylan)

Tracking

(Blocks: 1 bug)

Production
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
(Assignee)

Comment 1

2 years ago
Posted patch 1352264_draft_1.patch (obsolete) — Splinter Review
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.
(Assignee)

Updated

2 years ago
Blocks: 1351895
(Assignee)

Comment 2

2 years ago
Posted patch 1352264_1.patch (obsolete) — Splinter Review
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)
Attachment #8853186 - Attachment is obsolete: true
Attachment #8853623 - Flags: review?(glob)
This sounds like this is already being worked on, so marking P1.
Priority: -- → P1
(Assignee)

Comment 5

2 years ago
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 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 <gerv@gerv.net>                       
> +  #                 Ville SkyttŠ <ville.skytta@iki.fi>

this change is unrelated.

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

add a comment here, something like "trigger template preloading"
Attachment #8853623 - Flags: review?(glob) → review-
(Assignee)

Comment 7

2 years ago
> ::: extensions/BMO/template/en/default/bug/create/create-poweredby.html.tmpl
> @@ +17,4 @@
> >    # Rights Reserved.                                                          
> >    #                                                                           
> >    # Contributor(s): Gervase Markham <gerv@gerv.net>                       
> > +  #                 Ville SkyttŠ <ville.skytta@iki.fi>
> 
> 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.
(Assignee)

Updated

2 years ago
Priority: P1 → P2
(Assignee)

Comment 8

2 years ago
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
Attachment #8853623 - Attachment is obsolete: true
Attachment #8856707 - Flags: review?(glob)
Comment on attachment 8856707 [details] [diff] [review]
1352264_2.patch

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

r=glob
Attachment #8856707 - Flags: review?(glob) → review+
(Assignee)

Comment 10

2 years ago
To git@github.com:mozilla-bteam/bmo.git
   effcc51..c82963a  master -> master
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

2 years ago
This regresses rendering comment previews.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Depends on: 1361376
(Assignee)

Comment 12

2 years ago
To github.com:mozilla-bteam/bmo.git
   b1aece569..48cf53e5b  master -> master
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Blocks: 1368764
You need to log in before you can comment on or make changes to this bug.