js/src #includes are completely out of control

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(14 attachments)

3.77 KB, text/x-python-script
Details
3.26 KB, text/plain
Details
4.22 KB, patch
luke
: review+
Details | Diff | Splinter Review
14.24 KB, patch
luke
: review+
Details | Diff | Splinter Review
3.06 KB, patch
luke
: review+
Details | Diff | Splinter Review
558 bytes, patch
luke
: review+
Details | Diff | Splinter Review
1.78 KB, patch
luke
: review+
Details | Diff | Splinter Review
563 bytes, patch
luke
: review+
Details | Diff | Splinter Review
1.88 KB, patch
luke
: review+
Details | Diff | Splinter Review
4.45 KB, patch
luke
: review+
Details | Diff | Splinter Review
6.40 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.15 KB, patch
luke
: review+
Details | Diff | Splinter Review
17.68 KB, text/x-patch
Details
24.23 KB, patch
njn
: review+
Details | Diff | Splinter Review
vm/RegExpObject-inl.h #includes vm/RegExpStatics-inl.h
vm/RegExpStatics-inl.h #includes vm/RegExpObject-inl.h

That seems a bit senseless to me, but not as crazy as this:

jsinferinlines.h
      #includes vm/Stack-inl.h
which #includes ion/BaselineFrame-inl.h
which #includes vm/ScopeObject-inl.h
which #includes jsscriptinlines.h
which #includes vm/Shape-inl.h
which #includes jsobjinlines.h
which #includes jsinferinlines.h

I'm not sure why any of this works, but it's sick and it has got to stop. This is why incremental builds in js/src take so long. Not cycles specifically. But every header includes every other header. It's out of control.
or if you prefer: https://gist.github.com/jorendorff/5582667
Assignee: general → jorendorff
Posted file Current output
Line 24 of vm/Shape-inl.h says:
 #include "vm/Shape-inl.h"

I won't say who wrote that line of code, but the reviewer is one "jorendroff". :-P

https://hg.mozilla.org/mozilla-central/file/26ab72bfa9df/js/src/vm/Shape-inl.h#l24
Sorry for the spam, y'all, but I keep finding the most amazing symptoms of this problem.

jsinferinlines.h has the #ifndef jsinferinlines_h___ boilerplate in an unconventional place (*after* the includes rather than before) and if you move it to the conventional place, it actually fails to compile!!

Removing all the #include cycles will fix that for sure.
You're right it is out of control.  Nick was on his own warpath against this before he went on vacation; see discussion at the end of bug 634839.
OK. In that case I'll focus on cycles and the frontend stuff that is especially giving me grief at the moment. Big pile of patches incoming.
It's defined in a header, which creates annoying dependency edges. Fortunately it's only used in one .cpp file, so we can just move the definition there.
Attachment #752142 - Flags: review?(luke)
-inl headers are effectively .cpp files that happen to be #included elsewhere. It's backwards for an ordinary header file to include an -inl header, just as it would be backwards for a .h file to include a .cpp file.
Attachment #752144 - Flags: review?(luke)
BytecodeCompiler.h is meant to be a "façade" over the rest of the frontend. It's for code that just needs bog-standard frontend functionality. And that's everybody, except a few files that need to know about ParseNode, TokenStream, or source notes.

Touching Parser.h shouldn't make us recompile code that uses the façade.
Attachment #752145 - Flags: review?(luke)
...since that includes jsobjinlines.h, making a cycle.
Attachment #752146 - Flags: review?(luke)
...eliminating the #include cycle jsobjinlines.h -> jsscriptinlines.h -> vm/Shape-inl.h -> jsobjinlines.h.
Attachment #752149 - Flags: review?(luke)
...breaking the #include cycle jsobjinlines.h -> jsfuninlines.h -> vm/String-inl.h -> jsobjinlines.h.
Attachment #752226 - Flags: review?(luke)
Attachment #752142 - Flags: review?(luke) → review+
Attachment #752144 - Flags: review?(luke) → review+
Attachment #752145 - Flags: review?(luke) → review+
Attachment #752146 - Flags: review?(luke) → review+
Attachment #752148 - Flags: review?(luke) → review+
Attachment #752149 - Flags: review?(luke) → review+
Attachment #752150 - Flags: review?(luke) → review+
Attachment #752151 - Flags: review?(luke) → review+
Attachment #752226 - Flags: review?(luke) → review+
Nice work!  It'd be nice if we had some global metric (e.g., total number of #includes across an entire SM build or, more accurately, total number of bytes #included) so that we could measure the improvements of patch sets like this.
This makes a lot of files not include BytecodeEmitter.h (it was happening a lot via jsopcodeinlines.h).

Before this patch, touching Parser.h causes 39 .cpp files to be recompiled. With this patch, only 18.
Attachment #752748 - Flags: review?(luke) → review+
Last one in this bug, I promise.

Make files outside the frontend include frontend/BytecodeCompiler.h or frontend/SourceNotes.h if possible; those two act as facades and do not include all the rest of the frontend headers. With this change touching Parser.h only causes 11 .cpp files to recompile, mostly inside frontend.

Try servering. https://tbpl.mozilla.org/?tree=Try&rev=7c2c8bfd4b33
Attachment #755733 - Flags: review?(luke)
Comment on attachment 755733 [details] [diff] [review]
Part 12 - Isolate the frontend.

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

Stealing review.
Attachment #755733 - Flags: review?(luke) → review+
This is great stuff.  Thank you for doing it.  Please land it ASAP so I can continue removing unnecessary #includes with IWYU in bug 634839.

W.r.t. Luke's metrics point, https://bugzilla.mozilla.org/attachment.cgi?id=742099 is a script which adds a #warning to the top of every SpiderMonkey file, which lets you count how many times each one is read.  Bug 634839 comment 29 has some horrifying statistics.  I'll apply your patches (I'll have to redo my IWYU changes on top of them, but that's ok) and do some before/after measurements.

In general, I suspect we have far too much code in *inlines.h/*-inl.h files.  (Another gross example:  ion/LIR.h uses methods like toConstantIndex() which are defined in ion/LIR-inl.h;  I'm not even sure how that compiles.)

After I've finished with bug 634859, I plan to graph the #include dependencies.  I'm hoping that will make it easy to identify other improvements.
> After I've finished with bug 634859, I plan to graph the #include
> dependencies.  I'm hoping that will make it easy to identify other
> improvements.

I have a preliminary version of this working.  The graph is so big and interconnected that it's hard to gauge much.  Having said that, jsobjinlines.h appears to be key -- it's included by a lot of files, and includes a lot of files itself.  jscntxt.h, jsapi.h, and jsobj.h are all heavily connected as well.
Depends on: 879138
OK, more than half of this stack is passing Try Server:
  https://tbpl.mozilla.org/?tree=Try&rev=965c8a11e373
Marked "depends on" bug 879138 because, well, take a look. Feel free to steal review!
(In reply to Nicholas Nethercote [:njn] from comment #22)
> In general, I suspect we have far too much code in *inlines.h/*-inl.h files.
> (Another gross example:  ion/LIR.h uses methods like toConstantIndex() which
> are defined in ion/LIR-inl.h;  I'm not even sure how that compiles.)

Agreed. There's stuff like that everywhere. The compilers are pretty tolerant, but it's gotten to the point where it's surprisingly hard to change anything without breaking a build. The changes that got review here work fine in clang, but they break GCC; I spent a whole day fixing them to work in GCC (lots of compiles).
Whiteboard: [leave-open: huge difficult stack, half landed]
Landed 8 patches (actually 9 by mistake, but backed out the extra):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/5d6e363a7b4b

Pushed a few more to try server:
  https://tbpl.mozilla.org/?tree=Try&rev=d776acb7c031
I will test this on exotic archs like ppc & sparc64 once this is landed.
Bug 872416, part 6 - Make jsobjinlines.h not include jsscriptinlines.h, eliminating the #include cycle jsscriptinlines.h -> vm/Shape-inl.h -> jsobjinlines.h -> jsscriptinlines.h. NOT REVIEWED YET.

(In reply to Jason Orendorff [:jorendorff] from comment #27)
> Landed 8 patches (actually 9 by mistake, but backed out the extra):
>   https://hg.mozilla.org/integration/mozilla-inbound/rev/5d6e363a7b4b

That's the link of the last patch, and its commit msg ends with "NOT REVIEWED YET."  Whoops.

jorendorff, can you please email a single patch containing your not-yet-landed changes?  I'd like to start on some follow-up work.  Thanks!
> That's the link of the last patch, and its commit msg ends with "NOT
> REVIEWED YET."  Whoops.

Oh, you backed that one out.
I measured how often each header is #included before and after the patches that jorendorff landed.  It did improve things, reducing the total number of files #included from 146,478 to 135,534.  Here are the top 10s.

Before (133965:d94a84a5a0ac)
146478 counts:
( 1)   5360 ( 3.7%,  3.7%): #warning ./src/jscntxt.h
( 2)   4434 ( 3.0%,  6.7%): #warning ./src/jspubtd.h
( 3)   4394 ( 3.0%,  9.7%): #warning ./src/jsprvtd.h
( 4)   4296 ( 2.9%, 12.6%): #warning ./src/jsapi.h
( 5)   4204 ( 2.9%, 15.5%): #warning ./src/jsobj.h
( 6)   3885 ( 2.7%, 18.1%): #warning ./src/jstypes.h
( 7)   3319 ( 2.3%, 20.4%): #warning ./public/RootingAPI.h
( 8)   3294 ( 2.2%, 22.7%): #warning ./src/gc/Barrier.h
( 9)   3019 ( 2.1%, 24.7%): #warning ./public/Vector.h
(10)   2972 ( 2.0%, 26.7%): #warning ./public/HashTable.h

After (133975:04a12e995be8)
135534 counts:
( 1)   5187 ( 3.8%,  3.8%): #warning ./src/jscntxt.h
( 2)   4309 ( 3.2%,  7.0%): #warning ./src/jsapi.h
( 3)   4028 ( 3.0%, 10.0%): #warning ./src/jsobj.h
( 4)   3717 ( 2.7%, 12.7%): #warning ./src/jspubtd.h
( 5)   3699 ( 2.7%, 15.4%): #warning ./src/jsprvtd.h
( 6)   3511 ( 2.6%, 18.0%): #warning ./src/jstypes.h
( 7)   3319 ( 2.4%, 20.5%): #warning ./public/RootingAPI.h
( 8)   3294 ( 2.4%, 22.9%): #warning ./src/gc/Barrier.h
( 9)   2972 ( 2.2%, 25.1%): #warning ./public/HashTable.h
(10)   2962 ( 2.2%, 27.3%): #warning ./public/TemplateLib.h

It's a good sign.  I think these numbers can be reduced a lot if we keep chipping away.
jorendorff has moved to bug 879831 for the rest of this work.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open: huge difficult stack, half landed]
You need to log in before you can comment on or make changes to this bug.