js/src #includes are still out of control

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

15.24 KB, patch
njn
: review+
Details | Diff | Splinter Review
1.57 KB, patch
njn
: review+
Details | Diff | Splinter Review
1.75 KB, patch
njn
: review+
Details | Diff | Splinter Review
3.69 KB, patch
njn
: review+
Details | Diff | Splinter Review
27.51 KB, patch
njn
: review+
Details | Diff | Splinter Review
4.49 KB, patch
njn
: review+
Details | Diff | Splinter Review
26.15 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.55 KB, patch
njn
: review+
Details | Diff | Splinter Review
95.17 KB, patch
Details | Diff | Splinter Review
My stack from bug 872416 has evolved some, so I'm going to close that bug and ask for new reviews here. Fortunately the patches are still pretty trivial.

I really have to move on to other stuff, so I'm going to finish this one stack of patches and leave the rest to njn.
Assignee: general → jorendorff
Attachment #758739 - Flags: review?(n.nethercote)
Attachment #758737 - Attachment description: v1 → Part 1 - Uninline JSScript::sourceObject, v1
Attachment #758739 - Attachment description: v1 → part 2 - Move JSObject::asModule to jsobjinlines.h, v1
Attachment #758741 - Attachment description: v1 → Part 3 - Make jsobjinlines.h not include jsscriptinlines.h, eliminating the #include cycle jsscriptinlines.h -> vm/Shape-inl.h -> jsobjinlines.h -> jsscriptinlines.h, v1
Generally speaking, if you're nervous about the direction this is going (adding more -inl headers) you're not alone.

The reason for adding them, of course, is to remove dependencies from the non-inl headers, which aren't supposed to depend on -inl headers.

But note that at least Probes-inl.h and Debugger-inl.h are never included by other headers, only by .cpp files. So they don't make the knot any worse.
Attachment #758748 - Flags: review?(n.nethercote)
Basically just the patch in attachment 755733 [details] [diff] [review]. njn already reviewed it there and I've changed it a little but not significantly, so carrying over review.
Attachment #758751 - Flags: review+
Attachment #758737 - Flags: review?(n.nethercote) → review+
Comment on attachment 758739 [details] [diff] [review]
part 2 - Move JSObject::asModule to jsobjinlines.h, v1

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

In general I'm finding that moving things out of -inl.h files (either to .h or .cpp files) is virtuous for untangling the dependencies.  (And jsobjinlines.h is a real hotspot in the dependency graph that needs to be simplified.)

But I also understand that sometimes you've just gotta follow things all the way down the rabbit hole in order to satisfy a broader goal, so I'll trust you on this one.
Attachment #758739 - Flags: review?(n.nethercote) → review+
Comment on attachment 758741 [details] [diff] [review]
Part 3 - Make jsobjinlines.h not include jsscriptinlines.h, eliminating the #include cycle jsscriptinlines.h -> vm/Shape-inl.h -> jsobjinlines.h -> jsscriptinlines.h, v1

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

Excellent.
Attachment #758741 - Flags: review?(n.nethercote) → review+
Comment on attachment 758744 [details] [diff] [review]
Part 4 - Make vm/String-inl.h not include gc/Barrier-inl.h, breaking an #include cycle. Make vm/Shape-inl.h not #include itself. v1

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

::: js/src/jsobjinlines.h
@@ -1290,5 @@
>  inline bool
> -js::ObjectImpl::isProxy() const
> -{
> -    return js::IsProxy(const_cast<JSObject*>(this->asObjectPtr()));
> -}

I've been working on some changes to reduce the number of files that #include jsobjinlines.h and I moved this function into vm/ObjectImpl-inl.h too.  That's a good sign.
Attachment #758744 - Flags: review?(n.nethercote) → review+
I think that's enough for one bug.

This leaves a great many #include cycles in the codebase:

js/src/jsobjinlines.h
 -> js/src/jsfuninlines.h
     -> js/src/vm/ScopeObject-inl.h
         -> js/src/jsinferinlines.h
             -> js/src/vm/Stack-inl.h
                 -> js/src/ion/BaselineFrame-inl.h
                     -> js/src/vm/ScopeObject-inl.h
                 -> js/src/jsscriptinlines.h
                     -> js/src/vm/Shape-inl.h
                         -> js/src/jsobjinlines.h
                         -> js/src/vm/ScopeObject-inl.h
                 -> js/src/vm/ArgumentsObject-inl.h
                     -> js/src/vm/ScopeObject-inl.h
                 -> js/src/vm/ScopeObject-inl.h
         -> js/src/jsobjinlines.h
         -> js/src/jsscriptinlines.h
     -> js/src/vm/String-inl.h
         -> js/src/jsobjinlines.h
 -> js/src/jsinferinlines.h
 -> js/src/gc/Barrier-inl.h
     -> js/src/vm/String-inl.h
 -> js/src/vm/Shape-inl.h
 -> js/src/vm/String-inl.h

js/src/ion/LIR.h
 -> js/src/ion/x86/LIR-x86.h
     -> js/src/ion/LIR.h
 -> js/src/ion/x64/LIR-x64.h
     -> js/src/ion/LIR.h
 -> js/src/ion/arm/LIR-arm.h
     -> js/src/ion/LIR.h

Some obvious things we can do:

  - Move all class definitions from -inl headers to non-inl headers.
  - Move all forward declarations of classes and templates to jsprvtd.h.
  - Maybe bug 880041.

By the way: making vm/String-inl.h not include jsobjinlines.h would be a nice little win. It would break three cycles, and it *ought* to be easy -- nothing in String-inl.h actually directly uses anything defined in jsobjinlines.h! But actually removing that #include alone introduces a ton of GCC warnings and I think a compiler/linker error or two, because #include.
Comment on attachment 758746 [details] [diff] [review]
Part 5 - Factor out js::Probes inlines that depend on other "inlines" headers into vm/Probes-inl.h, v1

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

::: js/src/vm/Interpreter-inl.h
@@ +16,2 @@
>  #include "jsstr.h"
> +#include "ion/Ion.h"

There should be a blank line between the jsfoo.h headers and the foo/Bar.h headers.

::: js/src/vm/Probes.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "Probes-inl.h"

I'd be inclined to #include "Probes.h" here, and then #include "Probes-inl.h" in the usual spot near the bottom.  Also, can you make it "vm/Probes{.h,-inl.h}"?

::: js/src/vm/Stack.cpp
@@ +15,5 @@
>  #include "ion/IonFrames.h"
>  #include "ion/IonCompartment.h"
>  #endif
> +#include "vm/Stack.h"
> +#include "vm/ForkJoin.h"

Good -- I agree that omitting the directory names is confusing, and I want to move away from it, preferably by modifying the build parameters such that you can't omit them.  In the meantime, this is progress.

::: js/src/vm/Stack.h
@@ -13,2 @@
>  #include "ion/IonFrameIterator.h"
> -#endif

We seem to be very inconsistent about guarding Ion #includes.  Do you think we should not bother?  Perhaps this change should be saved for a follow-up that applies a new rule consistently.
Attachment #758746 - Flags: review?(n.nethercote) → review+
Attachment #758748 - Flags: review?(n.nethercote) → review+
> By the way: making vm/String-inl.h not include jsobjinlines.h would be a
> nice little win. It would break three cycles, and it *ought* to be easy

My in-progress patch that I mentioned in bug 880041 does this.  Your script tells me I've split the first clump into two:

js/src/gc/Barrier-inl.h
 -> js/src/vm/String-inl.h
     -> js/src/gc/Barrier-inl.h

js/src/jsobjinlines.h
 -> js/src/jsscriptinlines.h
     -> js/src/vm/Shape-inl.h
         -> js/src/vm/Shape-inl.h
         -> js/src/vm/ScopeObject-inl.h
             -> js/src/jsinferinlines.h
                 -> js/src/vm/Stack-inl.h
                     -> js/src/ion/BaselineFrame-inl.h
                         -> js/src/vm/ScopeObject-inl.h
                     -> js/src/jsscriptinlines.h
                     -> js/src/vm/ArgumentsObject-inl.h
                         -> js/src/vm/ScopeObject-inl.h
                     -> js/src/vm/ScopeObject-inl.h
             -> js/src/jsscriptinlines.h
             -> js/src/jsobjinlines.h
 -> js/src/jsinferinlines.h
 -> js/src/jsfuninlines.h
     -> js/src/vm/ScopeObject-inl.h
 -> js/src/vm/Shape-inl.h
BTW, I see your try run only included debug builds.  Please include opt builds as well!  I've frequently had opt-only breakage when working on #includes.
There was actually an opt-only failure, because a debug-only try run doesn't try to build with --disable-ion.

Fixed, trying again:

https://tbpl.mozilla.org/?tree=Try&rev=e7347b77dcb7
...by adding !defined(JS_ION) guard to all ion headers that are included from outside ion... except for ion/AsmJS.h and maybe one or two others where if you do that, something breaks.
Attachment #759494 - Flags: review?(n.nethercote)
Attachment #759494 - Flags: review?(n.nethercote) → review+
You need to log in before you can comment on or make changes to this bug.