Last Comment Bug 888768 - Minimize #include statements in SpiderMonkey (with the help of include-what-you-use)
: Minimize #include statements in SpiderMonkey (with the help of include-what-y...
Status: RESOLVED FIXED
[include-what-you-use]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Nicholas Nethercote [:njn]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: iwyu 902917
  Show dependency treegraph
 
Reported: 2013-06-30 16:11 PDT by Nicholas Nethercote [:njn]
Modified: 2013-11-06 21:58 PST (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(part 1) - Remove unnecessary js*inlines.h #includes, as found by include-what-you-use. (21.00 KB, patch)
2013-07-11 00:39 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
(part 1) - Remove unnecessary js*inlines.h #includes, as found by include-what-you-use. (18.51 KB, patch)
2013-07-11 11:26 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review
(part 2) - Remove frontend/SharedContext-inl.h. (4.02 KB, patch)
2013-07-11 18:00 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review
(part 3) - Remove unnecessary *-inl.h #includes, as found by include-what-you-use. (24.65 KB, patch)
2013-07-11 18:36 PDT, Nicholas Nethercote [:njn]
till: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2013-06-30 16:11:09 PDT
This bug will finish the job that bug 634839 started.
Comment 1 Nicholas Nethercote [:njn] 2013-07-11 00:39:47 PDT
Created attachment 773834 [details] [diff] [review]
(part 1) - Remove unnecessary js*inlines.h #includes, as found by include-what-you-use.

Mostly straightforward.  The only non-trivial bit is that I had to use
asJSContext() in a couple of places to get around the fact that
|GetNativeStackLimit(ExclusiveContext *cx)| is in jscntxtinlines.h.
Comment 2 Brian Hackett (:bhackett) 2013-07-11 05:07:50 PDT
Comment on attachment 773834 [details] [diff] [review]
(part 1) - Remove unnecessary js*inlines.h #includes, as found by include-what-you-use.

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

The issue with the asJSContext() calls is that while currently an ExclusiveContext is always a JSContext, pretty soon that will no longer be the case and these coercions will crash.  There are various spurious asJSContext() calls already, which are fixed by the patch in bug 892187 and which would be undone by these changes.
Comment 3 Nicholas Nethercote [:njn] 2013-07-11 11:26:48 PDT
Created attachment 774135 [details] [diff] [review]
(part 1) - Remove unnecessary js*inlines.h #includes, as found by include-what-you-use.

Aha.  

This patch is the same as the previous patch, but without the
asJSContext()/jscntxtninlines.h changes in ParseNode.cpp and FoldConstants.cpp.
Comment 4 Nicholas Nethercote [:njn] 2013-07-11 18:00:43 PDT
Created attachment 774391 [details] [diff] [review]
(part 2) - Remove frontend/SharedContext-inl.h.
Comment 5 Nicholas Nethercote [:njn] 2013-07-11 18:36:56 PDT
Created attachment 774406 [details] [diff] [review]
(part 3) - Remove unnecessary *-inl.h #includes, as found by include-what-you-use.

Note that I've been testing with --enable-gcgenerational, so I'm confident
about the changes within JSGC_GENERATIONAL guards.
Comment 6 Nicholas Nethercote [:njn] 2013-07-11 18:42:25 PDT
Part 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f30a9bb898f6
Comment 7 Till Schneidereit [till] (pto until Dec 5) 2013-07-12 05:42:23 PDT
Comment on attachment 774391 [details] [diff] [review]
(part 2) - Remove frontend/SharedContext-inl.h.

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

That was a lot of boiler-plate for a very simple line of code.
Comment 8 Till Schneidereit [till] (pto until Dec 5) 2013-07-12 06:06:19 PDT
Comment on attachment 774406 [details] [diff] [review]
(part 3) - Remove unnecessary *-inl.h #includes, as found by include-what-you-use.

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

Beautiful, would review again.

I added some "Ordering" comments and wanted to write something here about how these should be fixed if no script is forthcoming. Cleverly, I now consulted the Bugzilla and see that there is, in fact, a script forthcoming in bug 888088. Thus, let these comments be proof that I didn't just rubber-stamp the patch. Or something.

::: js/src/ion/AsmJSSignalHandlers.cpp
@@ +7,5 @@
>  #include "jscntxt.h"
>  
>  #include "ion/AsmJS.h"
>  #include "ion/AsmJSModule.h"
>  #include "assembler/assembler/MacroAssembler.h"

Ordering

::: js/src/ion/Bailouts.cpp
@@ +14,5 @@
>  #include "ion/Ion.h"
>  #include "ion/IonCompartment.h"
>  #include "ion/IonSpewer.h"
>  #include "vm/Interpreter.h"
>  #include "ion/BaselineJIT.h"

Ordering

::: js/src/ion/BaselineBailouts.cpp
@@ +8,5 @@
>  #include "ion/BaselineIC.h"
>  #include "ion/BaselineJIT.h"
>  #include "ion/CompileInfo.h"
>  #include "ion/IonSpewer.h"
>  #include "ion/IonFrames-inl.h"

Ordering

::: js/src/ion/BaselineCompiler.cpp
@@ +7,4 @@
>  #include "ion/BaselineJIT.h"
>  #include "ion/BaselineIC.h"
>  #include "ion/BaselineHelpers.h"
>  #include "ion/BaselineCompiler.h"

Ordering

::: js/src/ion/CodeGenerator.cpp
@@ +22,5 @@
>  #include "ion/ExecutionModeInlines.h"
>  #include "builtin/Eval.h"
>  #include "gc/Nursery.h"
>  #include "vm/ForkJoin.h"
>  #include "ion/ParallelSafetyAnalysis.h"

Ordering

::: js/src/ion/Ion.cpp
@@ -51,2 @@
>  #include "vm/Stack-inl.h"
> -#include "ion/IonFrames-inl.h"

Ordering

::: js/src/ion/VMFunctions.cpp
@@ -26,2 @@
>  #include "vm/Interpreter-inl.h"
> -#include "vm/StringObject-inl.h"

Ordering

::: js/src/jsopcode.cpp
@@ +33,5 @@
>  #include "frontend/BytecodeCompiler.h"
>  #include "frontend/SourceNotes.h"
>  #include "js/CharacterEncoding.h"
>  #include "vm/Shape.h"
> +#include "vm/ScopeObject.h"

Ordering
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-07-12 10:54:14 PDT
https://hg.mozilla.org/mozilla-central/rev/f30a9bb898f6
Comment 10 Nicholas Nethercote [:njn] 2013-07-12 12:58:12 PDT
> That was a lot of boiler-plate for a very simple line of code.

To be fair, it was simplified very recently, and until then it really did need to be in the -inl.h file.

> Beautiful, would review again.

Good!  Because I have more patches like this to come :)

W.r.t. ordering, as you say that's covered by bug 888088, and I think it's good to keep that separate from this bug.
Comment 13 Nicholas Nethercote [:njn] 2013-11-06 21:58:33 PST
I've done enough of this for now.  There may be more improvements to be made, but not in this bug.

Note You need to log in before you can comment on or make changes to this bug.