879831 - js/src #includes are still out of control

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - Uninline JSScript::sourceObject, v1

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Move JSObject::asModule to jsobjinlines.h, v1

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - Make jsobjinlines.h not include jsscriptinlines.h, eliminating the #include cycle jsscriptinlines.h -> vm/Shape-inl.h -> jsobjinlines.h -> jsscriptinlines.h, v1

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: 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

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 5 - Factor out js::Probes inlines that depend on other "inlines" headers into vm/Probes-inl.h, v1

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 6 - Factor out Debugger inlines that depend on other "inlines" headers into vm/Debugger-inl.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.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 7 - Isolate the frontend.

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.

Comment 8

[Nicholas Nethercote [inactive]]

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.

Comment 9

[Nicholas Nethercote [inactive]]

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.

Comment 10

[Nicholas Nethercote [inactive]]

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.

Comment 11

[Jason Orendorff [:jorendorff]]

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 12

[Nicholas Nethercote [inactive]]

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.

Comment 13

[Nicholas Nethercote [inactive]]

> 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

Comment 14

[Nicholas Nethercote [inactive]]

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.

Comment 15

[Jason Orendorff [:jorendorff]]

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

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: Part 0 - Fix no-ion build

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

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: omnibus patch for njn

Comment 18

[Jason Orendorff [:jorendorff]]

Trying again. https://tbpl.mozilla.org/?tree=Try&rev=b68567b42846

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3dba555bd153
https://hg.mozilla.org/mozilla-central/rev/e6ca293b6980
https://hg.mozilla.org/mozilla-central/rev/09f13d1498ee
https://hg.mozilla.org/mozilla-central/rev/2f5b1bd1ca45
https://hg.mozilla.org/mozilla-central/rev/2f0134cd42e2
https://hg.mozilla.org/mozilla-central/rev/8637d0b818fe
https://hg.mozilla.org/mozilla-central/rev/9d940604f3be
https://hg.mozilla.org/mozilla-central/rev/b252464d739e
https://hg.mozilla.org/mozilla-central/rev/8d444ac557a7