Closed Bug 1352907 Opened 3 years ago Closed 3 years ago

Simplify extension system for performance

Categories

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

Production
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dylan, Assigned: dylan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached file PR (obsolete) —
Attachment #8853833 - Flags: review?(glob)
Blocks: bmo-slow
Attached patch 1352907_1.patch (obsolete) — Splinter Review
Attachment #8853833 - Attachment is obsolete: true
Attachment #8853833 - Flags: review?(glob)
Attachment #8853980 - Flags: review?(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?
Attachment #8853980 - Flags: review?(glob) → review-
(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.
Depends on: 1355142
Attached patch 1352907_2.patchSplinter Review
This now depends on the bug that brings untaint().
Attachment #8853980 - Attachment is obsolete: true
Attachment #8856711 - Flags: review?(glob)
Comment on attachment 8856711 [details] [diff] [review]
1352907_2.patch

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

r=glob
Attachment #8856711 - Flags: review?(glob) → review+
To git@github.com:mozilla-bteam/bmo.git
   496cdc3..773dbde  master -> master
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.