If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

js/src #includes are still out of control

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla24
Points:
---

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
(Assignee)

Description

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

Comment 1

4 years ago
Created attachment 758737 [details] [diff] [review]
Part 1 - Uninline JSScript::sourceObject, v1
Attachment #758737 - Flags: review?(n.nethercote)
(Assignee)

Comment 2

4 years ago
Created attachment 758739 [details] [diff] [review]
part 2 - Move JSObject::asModule to jsobjinlines.h, v1
Assignee: general → jorendorff
Attachment #758739 - Flags: review?(n.nethercote)
(Assignee)

Comment 3

4 years ago
Created 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
Attachment #758741 - Flags: review?(n.nethercote)
(Assignee)

Updated

4 years ago
Attachment #758737 - Attachment description: v1 → Part 1 - Uninline JSScript::sourceObject, v1
(Assignee)

Updated

4 years ago
Attachment #758739 - Attachment description: v1 → part 2 - Move JSObject::asModule to jsobjinlines.h, v1
(Assignee)

Updated

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

Comment 4

4 years ago
Created 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
Attachment #758744 - Flags: review?(n.nethercote)
(Assignee)

Comment 5

4 years ago
Created attachment 758746 [details] [diff] [review]
Part 5 - Factor out js::Probes inlines that depend on other "inlines" headers into vm/Probes-inl.h, v1
Attachment #758746 - Flags: review?(n.nethercote)
(Assignee)

Comment 6

4 years ago
Created attachment 758748 [details] [diff] [review]
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.
Attachment #758748 - Flags: review?(n.nethercote)
(Assignee)

Comment 7

4 years ago
Created attachment 758751 [details] [diff] [review]
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.
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+
(Assignee)

Comment 11

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

Comment 15

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

Comment 16

4 years ago
Created attachment 759494 [details] [diff] [review]
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.
Attachment #759494 - Flags: review?(n.nethercote)
Attachment #759494 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 17

4 years ago
Created attachment 759509 [details] [diff] [review]
omnibus patch for njn
(Assignee)

Comment 18

4 years ago
Trying again. https://tbpl.mozilla.org/?tree=Try&rev=b68567b42846
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.