888768 - Minimize #include statements in SpiderMonkey (with the help of include-what-you-use)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

This bug will finish the job that bug 634839 started.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (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 [Laid off!]]

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 [inactive]]

Attachment: (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 [inactive]]

Attachment: (part 2) - Remove frontend/SharedContext-inl.h.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (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 [inactive]]

Part 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f30a9bb898f6

Comment 7

[Till Schneidereit [:till]]

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]]

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]]

https://hg.mozilla.org/mozilla-central/rev/f30a9bb898f6

Comment 10

[Nicholas Nethercote [inactive]]

> 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 11

[Nicholas Nethercote [inactive]]

Parts 2 and 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78b3873cd79e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e54e7b5bf27d

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/78b3873cd79e
https://hg.mozilla.org/mozilla-central/rev/e54e7b5bf27d

Comment 13

[Nicholas Nethercote [inactive]]

I've done enough of this for now.  There may be more improvements to be made, but not in this bug.