Closed
Bug 872416
Opened 12 years ago
Closed 12 years ago
js/src #includes are completely out of control
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
Attachments
(14 files)
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
|
n.nethercote
:
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.
Assignee | ||
Comment 1•12 years ago
|
||
or if you prefer: https://gist.github.com/jorendorff/5582667
Assignee: general → jorendorff
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
-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)
Assignee | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Comment 10•12 years ago
|
||
...since that includes jsobjinlines.h, making a cycle.
Attachment #752146 -
Flags: review?(luke)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #752148 -
Flags: review?(luke)
Assignee | ||
Comment 12•12 years ago
|
||
...eliminating the #include cycle jsobjinlines.h -> jsscriptinlines.h -> vm/Shape-inl.h -> jsobjinlines.h.
Attachment #752149 -
Flags: review?(luke)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #752150 -
Flags: review?(luke)
Assignee | ||
Comment 14•12 years ago
|
||
...breaking two more #include cycles.
Attachment #752151 -
Flags: review?(luke)
Assignee | ||
Comment 15•12 years ago
|
||
...breaking the #include cycle jsobjinlines.h -> jsfuninlines.h -> vm/String-inl.h -> jsobjinlines.h.
Attachment #752226 -
Flags: review?(luke)
Updated•12 years ago
|
Attachment #752142 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #752144 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #752145 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #752146 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #752148 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #752149 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #752150 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #752151 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #752226 -
Flags: review?(luke) → review+
Comment 16•12 years ago
|
||
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.
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #752748 -
Flags: review?(luke)
Assignee | ||
Comment 18•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #752748 -
Flags: review?(luke) → review+
Assignee | ||
Comment 19•12 years ago
|
||
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)
Assignee | ||
Comment 20•12 years ago
|
||
Trying again. https://tbpl.mozilla.org/?tree=Try&rev=c38f68d528f1
Comment 21•12 years ago
|
||
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+
Comment 22•12 years ago
|
||
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•12 years ago
|
||
> 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.
Assignee | ||
Comment 24•12 years ago
|
||
OK, more than half of this stack is passing Try Server:
https://tbpl.mozilla.org/?tree=Try&rev=965c8a11e373
Assignee | ||
Comment 25•12 years ago
|
||
Marked "depends on" bug 879138 because, well, take a look. Feel free to steal review!
Assignee | ||
Comment 26•12 years ago
|
||
(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).
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave-open: huge difficult stack, half landed]
Assignee | ||
Comment 27•12 years ago
|
||
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•12 years ago
|
||
I will test this on exotic archs like ppc & sparc64 once this is landed.
Comment 29•12 years ago
|
||
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•12 years ago
|
||
> 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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
jorendorff has moved to bug 879831 for the rest of this work.
Status: NEW → RESOLVED
Closed: 12 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.
Description
•