The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [include-what-you-use])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
This bug will finish the job that bug 634839 started.
(Assignee)

Updated

4 years ago
Whiteboard: [include-what-you-use][leave open] → [include-what-you-use]
(Assignee)

Comment 1

4 years ago
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.
Attachment #773834 - Flags: review?(bhackett1024)
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.
Attachment #773834 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

4 years ago
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.
Attachment #774135 - Flags: review?(bhackett1024)
(Assignee)

Updated

4 years ago
Attachment #773834 - Attachment is obsolete: true
Attachment #774135 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 4

4 years ago
Created attachment 774391 [details] [diff] [review]
(part 2) - Remove frontend/SharedContext-inl.h.
Attachment #774391 - Flags: review?(till)
(Assignee)

Comment 5

4 years ago
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.
Attachment #774406 - Flags: review?(till)
(Assignee)

Comment 6

4 years ago
Part 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f30a9bb898f6
Whiteboard: [include-what-you-use] → [include-what-you-use][leave open]
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.
Attachment #774391 - Flags: review?(till) → review+
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
Attachment #774406 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/f30a9bb898f6
(Assignee)

Comment 10

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

Comment 11

4 years ago
Parts 2 and 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78b3873cd79e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e54e7b5bf27d
https://hg.mozilla.org/mozilla-central/rev/78b3873cd79e
https://hg.mozilla.org/mozilla-central/rev/e54e7b5bf27d
(Assignee)

Updated

4 years ago
Whiteboard: [include-what-you-use][leave open] → [include-what-you-use]
Target Milestone: --- → mozilla25
(Assignee)

Updated

4 years ago
Blocks: 902917

Updated

4 years ago
Blocks: 903843
(Assignee)

Comment 13

3 years ago
I've done enough of this for now.  There may be more improvements to be made, but not in this bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.