872416 - js/src #includes are completely out of control

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Python 3 script to find #include cycles in js/src

or if you prefer: https://gist.github.com/jorendorff/5582667

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Current output

Comment 3

[Jason Orendorff [:jorendorff]]

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

Comment 4

[Jason Orendorff [:jorendorff]]

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.

Comment 5

[Luke Wagner [:luke]]

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.

Comment 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - move LexicalLookup

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.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Make jsanalyze.h not include jsopcodeinlines.h

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

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - Make BytecodeCompiler.h not include Parser.h

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.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Part 4 - Make jsobjinlines.h not include builtin/Iterator-inl.h

...since that includes jsobjinlines.h, making a cycle.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Part 5 - Make objinlines.h not include vm/RegExpStatics-inl.h.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Part 6 - Make jsscriptinlines.h not include vm/Shape-inl.h

...eliminating the #include cycle jsobjinlines.h -> jsscriptinlines.h -> vm/Shape-inl.h -> jsobjinlines.h.

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: Part 7 - Make vm/String-inl.h and gc/Barrier-inl.h not include each other, breaking an #include cycle. Make vm/Shape-inl.h not #include itself.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: Part 8 - Make jsobjinlines.h not include vm/ObjectImpl-inl.h, vm/Shape-inl.h, or vm/String-inl.h.

...breaking two more #include cycles.

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: Part 9 - Make jsobjinlines.h not include jsfuninlines.h

...breaking the #include cycle jsobjinlines.h -> jsfuninlines.h -> vm/String-inl.h -> jsobjinlines.h.

Comment 16

[Luke Wagner [:luke]]

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.

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: Part 10 - Make vm/RegExpObject-inl.h and vm/RegExpStatic-inl.h not include each other, breaking an include cycle.

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: Part 11 - Factor out SourceNotes.h

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.

Comment 19

[Jason Orendorff [:jorendorff]]

Attachment: Part 12 - Isolate the frontend.

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

Comment 20

[Jason Orendorff [:jorendorff]]

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

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 755733 [details] [diff] [review]
Part 12 - Isolate the frontend.

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

Stealing review.

Comment 22

[Nicholas Nethercote [inactive]]

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.

Comment 23

[Nicholas Nethercote [inactive]]

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

Comment 24

[Jason Orendorff [:jorendorff]]

OK, more than half of this stack is passing Try Server:
  https://tbpl.mozilla.org/?tree=Try&rev=965c8a11e373

Comment 25

[Jason Orendorff [:jorendorff]]

Marked "depends on" bug 879138 because, well, take a look. Feel free to steal review!

Comment 26

[Jason Orendorff [:jorendorff]]

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

Comment 27

[Jason Orendorff [:jorendorff]]

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

Comment 28

[Landry Breuil (:gaston)]

I will test this on exotic archs like ppc & sparc64 once this is landed.

Comment 29

[Nicholas Nethercote [inactive]]

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!

Comment 30

[Nicholas Nethercote [inactive]]

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

Oh, you backed that one out.

Comment 31

[Nicholas Nethercote [inactive]]

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.

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8ed346e399fe
https://hg.mozilla.org/mozilla-central/rev/c2e714bcb50d
https://hg.mozilla.org/mozilla-central/rev/88b70ed16f45
https://hg.mozilla.org/mozilla-central/rev/5f70ad99154c
https://hg.mozilla.org/mozilla-central/rev/163d6fb6edd5
https://hg.mozilla.org/mozilla-central/rev/bcf7a837657c
https://hg.mozilla.org/mozilla-central/rev/0f50e301b1c4

Comment 33

[Nicholas Nethercote [inactive]]

jorendorff has moved to bug 879831 for the rest of this work.