1352907 - Simplify extension system for performance

Status: RESOLVED FIXED

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

Description

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

The extension system spends a lot of time on concerns we don't have.
We only care about extensions in the extension directory -- nothing else is
used. Supporting all the variations of how extensions can be loaded causes it to
be slow.

It also causes loading regular modules to be slow, because it adds
and @INC hook for every extension.

This commit uses a single @INC hook and makes some assumptions
about how extensions are organized. As a result it is much faster.

Comment 1

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

Attachment: PR

Comment 2

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

Attachment: 1352907_1.patch

Comment 3

[:glob ✱]

Comment on attachment 8853980 [details] [diff] [review]
1352907_1.patch

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

this looks like a great improvement, just a few minor things to fix up

::: Bugzilla/Extension.pm
@@ +19,5 @@
>  use File::Spec;
>  
> +BEGIN {
> +    eval {
> +        die;

i suspect this 'die' shouldn't be here

@@ +21,5 @@
> +BEGIN {
> +    eval {
> +        die;
> +        require Taint::Util;
> +        Taint::Util->import('untaint');

it would be better to wait for bug 1351895 to land which adds Taint::Util

@@ +33,5 @@
> +    };
> +}
> +
> +
> +my @EXTENSIONS;

could this be a 'state' variable in load_all?

Comment 4

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

(In reply to Byron Jones ‹:glob› from comment #3)
> Comment on attachment 8853980 [details] [diff] [review]
> 1352907_1.patch
> 
> Review of attachment 8853980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this looks like a great improvement, just a few minor things to fix up
> 
> ::: Bugzilla/Extension.pm
> @@ +19,5 @@
> >  use File::Spec;
> >  
> > +BEGIN {
> > +    eval {
> > +        die;
> 
> i suspect this 'die' shouldn't be here

In the past week, I have used:

1) debugging prints
2) debugging screenshots
and finally:

3) debugging exceptions

Yes, I'll remove that entire thing for the next iteration.

> @@ +21,5 @@
> > +BEGIN {
> > +    eval {
> > +        die;
> > +        require Taint::Util;
> > +        Taint::Util->import('untaint');
> 
> it would be better to wait for bug 1351895 to land which adds Taint::Util
Yes, that makes sense.

> 
> @@ +33,5 @@
> > +    };
> > +}
> > +
> > +
> > +my @EXTENSIONS;
> 
> could this be a 'state' variable in load_all?
It can be if it's a reference. Only scalars can be state variables.

Comment 5

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

Attachment: 1352907_2.patch

This now depends on the bug that brings untaint().

Comment 6

[:glob ✱]

Comment on attachment 8856711 [details] [diff] [review]
1352907_2.patch

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

r=glob

Comment 7

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

To [e-mail]:mozilla-bteam/bmo.git
   496cdc3..773dbde  master -> master