Closed
Bug 1352907
Opened 7 years ago
Closed 7 years ago
Simplify extension system for performance
Categories
(bugzilla.mozilla.org :: General, enhancement)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dylan, Assigned: dylan)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
16.21 KB,
patch
|
glob
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Attachment #8853833 -
Flags: review?(glob)
Assignee | ||
Comment 2•7 years ago
|
||
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•7 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 | ||
Comment 5•7 years ago
|
||
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•7 years ago
|
||
To git@github.com:mozilla-bteam/bmo.git 496cdc3..773dbde master -> master
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•