Simplify extension system for performance

RESOLVED FIXED

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dylan, Assigned: dylan)

Tracking

(Blocks: 1 bug)

Production
Dependency tree / graph

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8853833 [details] [review]
PR
Attachment #8853833 - Flags: review?(glob)
(Assignee)

Updated

2 years ago
Blocks: 1351895
(Assignee)

Comment 2

2 years ago
Created attachment 8853980 [details] [diff] [review]
1352907_1.patch
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-
(Assignee)

Comment 4

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

Updated

2 years ago
Depends on: 1355142
(Assignee)

Comment 5

2 years ago
Created attachment 8856711 [details] [diff] [review]
1352907_2.patch

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

Comment 7

2 years ago
To git@github.com:mozilla-bteam/bmo.git
   496cdc3..773dbde  master -> master
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.