634839 - Try "include-what-you-use" on SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

http://code.google.com/p/include-what-you-use/ is a tool that tells you which #includes aren't necessary, potentially leading to better compile times.  It'd be good to try this on SpiderMonkey to see if it works and what it can tell us.

This is related to bug 558723.

Comment 1

[Sander Mathijs van Veen]

1. Compiling "include-what-you-use"
svn co http://llvm.org/svn/llvm-project/llvm/trunk llvm
cd llvm/tools/
svn co http://llvm.org/svn/llvm-project/cfe/trunk clang
cd clang/tools/
svn checkout http://include-what-you-use.googlecode.com/svn/trunk/ include-what-you-use
cd ../../../
./configure --enable-optimized
make -j4
cd tools/clang/tools/include-what-you-use
make -j4

The flags "-j4" for make and "--enable-optimized" for "./configure" are not necessary, but I used it to speed up the compilation of clang itself and speed up the compilation of SpiderMonkey using clang. Note: If you do not use "--enable-optimized", you should change "Release+Asserts" in "Debug+Asserts" in the command line used below.

2. Invocation of "include-what-you-use" on SpiderMonkey
make -k CXX=/path/to/llvm/Release+Asserts/bin/include-what-you-use

The "-k" flag (continue after failure) is important to add, or "include-what-you-use" will fail after the first file, since "include-what-you-use" does not produce the target required by the Makefile.

3. Results so far
Unfortunately, "include-what-you-use" does not work out-of-the-box on SpiderMonkey. It analyses the first and second file "jscpucfg.cpp" and "jskwgen.cpp" succesfully. After the analysis of "jskwgen.cpp" is done, Make quits:

make[1]: *** [host_jskwgen.o] Error 1
make[1]: Target `export' not remade because of errors.
make[1]: Target `export' not remade because of errors.
make: *** [default] Error 2

I created the following workaround (which should be fixed to make include-what-you-use effective):

make jscpucfg
make host_jskwgen
make -k CXX=/home/sander/work/llvm/Release+Asserts/bin/include-what-you-use 1> /dev/null
make host_jsoplengen
make -k CXX=/home/sander/work/llvm/Release+Asserts/bin/include-what-you-use 1> /dev/null 2> iwyu.log

The log file "iwyu.log" (see attachment) is rather large, but contains a lot of information about #includes which are not used. I was unable to separate the clang compiler warnings and iwyu messages.

4. Unstable "include-what-you-use"
Notice that iwyu is work in progress and considered `alpha' software. You'll see segmentation faults in the log file, for example: make[1]: *** [jsscript.o] Segmentation fault (core dumped). I tracked it down to:
Core was generated by `/home/sander/work/llvm/Release+Asserts/bin/include-what-you-use -o jsscript.o -'.
Program terminated with signal 11, Segmentation fault.
#0  0x084fd73a in (anonymous namespace)::CGObjCMac::EmitTryOrSynchronizedStmt(clang::CodeGen::CodeGenFunction&, clang::Stmt const&) ()

I'm looking forward to any suggestions about how to fix these segmentation faults and/or a better solution for the iwyu workaround.

Comment 2

[Sander Mathijs van Veen]

Attachment: include-what-you-use log file on js/src (compressed)

Comment 3

[Nicholas Nethercote [inactive]]

Sander, this is great work, thanks for doing it.

The next step is to try to act on the info that IWYU has found.  Here's what I'd do:

- Read bug 558723 to understand how we want our #includes to be structured.  I don't remember all the details but I know there's a certain order we want (alphabetical, but list <foo.h> headers before "foo.h" headers, and "fooinlines.h" headers after that, maybe some other things).

- Try changing files one at a time, making sure it compiles, you don't get warnings, etc.  I'd just try using the list that IWYU suggests for each file, but in the required order as described above.

- Once you've changed most/all of them, see if it speeds up compilation.  First, it should speed up compilation of each individual file, because there'll be no unneeded #includes and therefore less code to process.  Second, it should speed up builds when changes have been made, because make won't end up rebuilding files based on unneeded dependencies.  The first of these is easier to measure than the second -- just compare from-scratch build times.

Hopefully that seems clear!  Please ask if you hit any problems.

Comment 4

[Nicholas Nethercote [inactive]]

BTW, it's interesting to see just how bad our #include lisst are.  Eg. here's the output for jsscope.h:


/home/sander/fx6/js/src/jsscope.cpp should add these lines:
#include <stddef.h>                     // for size_t
#include "js-config.h"                  // for JS_BYTES_PER_WORD, etc
#include "jscntxtinlines.h"             // for LeaveTraceIfGlobalObject
#include "jscompartment.h"              // for JS_PROPERTY_TREE, etc
#include "jscompat.h"                   // for uintN, intN
#include "jsgc.h"                       // for TriggerGC
#include "jshash.h"                     // for JSHashNumber, etc
#include "jsotypes.h"                   // for uint32, uint8, int16
#include "jspropertycache.h"            // for SHAPE_OVERFLOW_BIT
#include "jspropertytree.h"             // for PropertyTree, etc
#include "jsval.h"                      // for jsid, JSID_BITS
#include "jsvalue.h"                    // for PropertyOp, etc

/home/sander/fx6/js/src/jsscope.cpp should remove these lines:
- #include <string.h>  // lines 46-46
- #include "jsarena.h"  // lines 49-49
- #include "jsatom.h"  // lines 55-55
- #include "jsclist.h"  // lines 51-51
- #include "jsdbgapi.h"  // lines 57-57
- #include "jsfun.h"  // lines 58-58
- #include "jsnum.h"  // lines 60-60
- #include "jsstdint.h"  // lines 48-48
- #include "jsstr.h"  // lines 63-63

The full include-list for /home/sander/fx6/js/src/jsscope.cpp:
#include <stddef.h>                     // for size_t
#include <stdlib.h>                     // for NULL
#include <new>                          // for operator new
#include "js-config.h"                  // for JS_BYTES_PER_WORD, etc
#include "jsapi.h"                      // for JSPROP_SHARED, etc
#include "jsbit.h"                      // for JS_CeilingLog2
#include "jscntxt.h"                    // for JSContext, JSRuntime, etc
#include "jscntxtinlines.h"             // for LeaveTraceIfGlobalObject
#include "jscompartment.h"              // for JS_PROPERTY_TREE, etc
#include "jscompat.h"                   // for uintN, intN
#include "jsdbgapiinlines.h"
#include "jsdhash.h"                    // for JS_DHASH_BITS
#include "jsgc.h"                       // for TriggerGC
#include "jshash.h"                     // for JSHashNumber, etc
#include "jslock.h"                     // for JS_ATOMIC_INCREMENT
#include "jsobj.h"                      // for JSObject, JSSLOT_FREE, etc
#include "jsobjinlines.h"               // for JSObject::inDictionaryMode, etc
#include "jsotypes.h"                   // for uint32, uint8, int16
#include "jspropertycache.h"            // for SHAPE_OVERFLOW_BIT
#include "jspropertytree.h"             // for PropertyTree, etc
#include "jsscope.h"                    // for Shape, PropertyTable, etc
#include "jsscopeinlines.h"             // for Shape::Shape, etc
#include "jstracer.h"                   // for TraceRecorder
#include "jstypes.h"                    // for JS_BIT, JS_BITMASK
#include "jsutil.h"                     // for JS_ASSERT, JS_ASSERT_IF, etc
#include "jsval.h"                      // for jsid, JSID_BITS
#include "jsvalue.h"                    // for PropertyOp, etc


There are 9 headers included unnecessarily!  But there are 12 that should be added, ie. are currently boot-legged (ie. they are #included indirectly but they should be #included directly because something they define is used in jsscope.cpp).

Comment 5

[Sander Mathijs van Veen]

Attachment: include-what-you-use patch for js/src

Comment 6

[Nicholas Nethercote [inactive]]

Sander, how did you generate that patch?  The filenames aren't showing up in the 
"diff" view in Bugzilla.  Looking at the patch, each file's changes are introduced by this:

  --- a/jskwgen.cpp
  +++ b/jskwgen.cpp

but the diffs produced by Mercurial look like this:

  diff --git a/js/src/jsscan.cpp b/js/src/jsscan.cpp
  --- a/js/src/jsscan.cpp
  +++ b/js/src/jsscan.cpp

I have this in my .hgrc file

  [defaults]
  diff=-p -U 8

https://developer.mozilla.org/en/Creating_a_patch might be useful here.

Apart from that minor problem, the patch basically looks good;  I'll look at it carefully on Monday.  Thanks!

Comment 7

[Sander Mathijs van Veen]

Attachment: include-what-you-use patch for js/src

This time I used the normal way of creating a patch (using hg diff).

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 535817 [details] [diff] [review]
include-what-you-use patch for js/src

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

I'm giving a "r-", because I found two problems with this patch, described below.  But don't be discouraged, that's common for a first patch and it should be easy to fix them.  Once you've done that, please post an updated patch.  Thanks!

::: js/src/jsfriendapi.cpp
@@ -38,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
>  #include "jscntxt.h"
> -#include "jscompartment.h"
> -#include "jsfriendapi.h"

This is weird.  jsfriendapi.h declares various functions (eg. JS_GetAnonymousString) and jsfriendapi.cpp defines them.  So jsfriendapi.cpp should definitely #include jsfriendapi.h, and in general a.cpp should always include a.h.  (I guess we really want "include-what-you-use-and-define" not just "include-what-you-use".)

::: js/src/jsgcstats.cpp
@@ -43,5 @@
> -#include "jsxml.h"
> -#include "jsbuiltins.h"
> -#include "jscompartment.h"
> -
> -#include "jsgcinlines.h"

I just realized that you removed all the headers that IWYU said you could, but didn't add the ones it said you should.  For example, for this file it said the full list should be this:

#include <stddef.h>
#include "js-confdefs.h"
#include "jscntxt.h"
#include "jsgcstats.h"
#include "jsotypes.h"
#include "jspubtd.h"
#include "jstypes.h"
#include "jsutil.h"
#include "prmjtime.h"  

This is required to avoid the "no bootlegging" principle described in bug 558723.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

You should not be including js-confdefs.h directly.  That's generated by the build system from all of the AC_DEFINE()s in configure and the build system includes that header on the command line.

Comment 10

[Tom S [:evilpie]]

Somebody should follow up with this guy.

Comment 11

[Sander Mathijs van Veen]

Attachment: iwyu patch for js/src (mozilla-inbound)

Applied include-what-you-want's recommendations on js/src in repository mozilla-inbound. I modified fix_includes (part of iwyu) to trigger callbacks, which will follow some logic specific to the mozilla code base (e.g. *inlines.h should not be mixed with the js*.h includes), so I was able to automate the process of applying the recommendations.

The script fix_include messed up some files, which were at that moment not easily fixed, so I temporarily ignored those files (see OMIT_FILES). 

# omit jsatom.cpp because fix_includes removes the second jsproto.tbl.
# omit jsarena.cpp because of an undefined reference to `ubound'.
# jsobj.cpp should not include jsobjinlines.h, but vm/String-inl.h should.
# omit jsscan.cpp because fix_includes removes the second jsproto.tbl.

# Ignore iwyu recommendations for the matching files completely
OMIT_FILES = re.compile(r'(js(atom|arena|opcode|scan|xml|tracer)\.cpp' \
                         '|nanojit/.*' \
                         '|methodjit/InvokeHelpers.cpp' \
                         '|v8-dtoa/.*' \
                         '|shell/js.cpp)$')

# Ignore addition of the matching include-statements
OMIT_INCLUDES = re.compile(r'"(dist/.*' \
                            '|jsotypes.h' \
                            '|js-conf(ig|defs)\.h' \
                            '|js.*\.msg' \
                            '|.*\.tbl)"')

# Ignore removal of the matching include-statements
OMIT_REMOVAL = re.compile(r'"(dist/.*' \
                            '|jsautokw.h' \
                            '|js-conf(ig|defs)\.h' \
                            '|js.*\.msg' \
                            '|.*\.tbl)"')

# Ignore removal of include-statements for the matching files
LOCAL_OMIT_REMOVAL = {
        'jsatom.cpp': re.compile(r'"(vm/(GlobalObject|String-inl)).h"'),
        'jscntxt.cpp': re.compile(r'"(assembler/assembler/MacroAssembler' \
                                   '|jsscan|vm/(GlobalObject|String-inl)).h"'),
        'jsdbgapi.cpp': re.compile(r'"(jsemit).h"'),
        'jsinterp.cpp': re.compile(r'"(jsemit|jsgcmark).h"'),
        'jsiter.cpp': re.compile(r'"(jsscan).h"'),
        'jsprf.cpp': re.compile(r'"(jsstdint).h"'),
        'jsutil.cpp': re.compile(r'"(jsgcmark).h"'),
        'jsgcmark.cpp': re.compile(r'"(jsgcmark).h"'),
        'jsweakmap.cpp': re.compile(r'"(jsgcmark).h"'),
        'perf/pm_linux.cpp': re.compile(r'<new>'),
        }

So, there are still files which are not handled yet. I'll take a look at those files in the following days.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 551376 [details] [diff] [review]
iwyu patch for js/src (mozilla-inbound)

>--- a/js/src/assembler/jit/ExecutableAllocatorOS2.cpp	Sat Aug 06 14:04:58 2011 -0400
>+++ b/js/src/assembler/jit/ExecutableAllocatorOS2.cpp	Mon Aug 08 03:42:12 2011 +0200
>-#include "ExecutableAllocator.h"
>-
>+#include "assembler/wtf/Platform.h"
> #if ENABLE_ASSEMBLER && WTF_OS_OS2
> 
> #define INCL_DOS
> #include <os2.h>
> 
> namespace JSC {
> 
> size_t ExecutableAllocator::determinePageSize()

>--- a/js/src/assembler/jit/ExecutableAllocatorPosix.cpp	Sat Aug 06 14:04:58 2011 -0400
>+++ b/js/src/assembler/jit/ExecutableAllocatorPosix.cpp	Mon Aug 08 03:42:12 2011 +0200
>+#include <stddef.h>
>+
> #include "ExecutableAllocator.h"
>+#include "assembler/wtf/Assertions.h"
>+#include "assembler/wtf/Platform.h"
> 
> #if ENABLE_ASSEMBLER && WTF_OS_UNIX && !WTF_OS_SYMBIAN
> 
> #include <sys/mman.h>
> #include <unistd.h>
> #include <wtf/VMTags.h>
> 
> namespace JSC {

>--- a/js/src/assembler/jit/ExecutableAllocatorWin.cpp	Sat Aug 06 14:04:58 2011 -0400
>+++ b/js/src/assembler/jit/ExecutableAllocatorWin.cpp	Mon Aug 08 03:42:12 2011 +0200
>-#include "ExecutableAllocator.h"
>-
>+#include "assembler/wtf/Platform.h"
> #if ENABLE_ASSEMBLER && WTF_OS_WINDOWS
> 
> #include "jswin.h"
> 
> namespace JSC {
> 
> size_t ExecutableAllocator::determinePageSize()
> {

Looks like you're on POSIX, but not everyone is, unfortunately.

>--- a/js/src/frontend/ParseMaps.h	Sat Aug 06 14:04:58 2011 -0400
>+++ b/js/src/frontend/ParseMaps.h	Mon Aug 08 03:42:12 2011 +0200
> namespace js {
> 
> /*
>  * A pool that permits the reuse of the backing storage for the defn, index, or
>  * defn-or-header (multi) maps.
>  *
>  * The pool owns all the maps that are given out, and is responsible for
>  * relinquishing all resources when |purgeAll| is triggered.
>  */
>+class MultiDeclRange;
>+namespace tl {
>+template <class T> struct IsPodType;
>+}  // namespace tl
>+
> class ParseMapPool

I think these declarations want to be before the comment.

>--- a/js/src/jsapi.cpp	Sat Aug 06 14:04:58 2011 -0400
>+++ b/js/src/jsapi.cpp	Mon Aug 08 03:42:12 2011 +0200
>+#include "vm/String.h"
>+#include "vm/String-inl.h"
>+
>+#include "jscntxtinlines.h"

-inl should be after inlines, AIUI.

>-#if ENABLE_YARR_JIT
>-#include "assembler/jit/ExecutableAllocator.h"
>-#include "methodjit/Logging.h"
>-#endif

You seem to be losing this #if.

>--- a/js/src/jsemit.cpp	Sat Aug 06 14:04:58 2011 -0400
>+++ b/js/src/jsemit.cpp	Mon Aug 08 03:42:12 2011 +0200
>+#include "jsautooplen.h"        // generated headers last

Not anymore, I don't think.

>--- a/js/src/jsproxy.cpp	Sat Aug 06 14:04:58 2011 -0400
>+++ b/js/src/jsproxy.cpp	Mon Aug 08 03:42:12 2011 +0200
>+#include "jsval.h"
>+#include "jsvalue.h"
>+
>+class JSAtom;
>+class JSString;
> 
> #include "jsatominlines.h"

I wonder how it decides where to put forward declarations.

>--- a/js/src/jsreflect.cpp	Sat Aug 06 14:04:58 2011 -0400
>+++ b/js/src/jsreflect.cpp	Mon Aug 08 03:42:12 2011 +0200
> #include <string.h>     /* for jsparse.h */
>+#include "jscntxt.h"    /* for jsparse.h */

These comments can go now, I hope?

Comment 13

[Sander Mathijs van Veen]

Attachment: iwyu patch for js/src (mozilla-inbound)

Removed whitespace fixes from the patch (hg diff -w) and applied the recommendations from Ms2ger on the patch.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 551897 [details] [diff] [review]
iwyu patch for js/src (mozilla-inbound)

>--- a/js/src/jsarray.cpp	Sat Aug 06 14:04:58 2011 -0400
>+++ b/js/src/jsarray.cpp	Tue Aug 09 23:33:06 2011 +0200
> #include "vm/ArgumentsObject.h"
>+#include "vm/ArgumentsObject-inl.h"
>+#include "vm/GlobalObject.h"
>+#include "vm/Stack.h"
>+#include "vm/Stack-inl.h"
>+#include "vm/String.h"
>+
>+struct JSHashEntry;
> 
> #include "jsatominlines.h"
>-#include "jscntxtinlines.h"
> #include "jsinterpinlines.h"
> #include "jsobjinlines.h"
> #include "jsstrinlines.h"

You've still got -inl.h intermixed with normal includes, and forward declarations between includes. It would be great if you could fix that.

Comment 15

[Sander Mathijs van Veen]

Attachment: Applied iwyu on js/src (repository mozilla-inbound)

I wasn't sure about the removal of the following lines:

In js/src/jsfun.cpp:

-#ifdef JS_METHODJIT
-#include "methodjit/MethodJIT.h"
-#endif

In js/src/jsinterp.cpp:

-#if defined(JS_METHODJIT) && defined(JS_MONOIC)
-#include "methodjit/MonoIC.h"
-#endif

In js/src/jsparse.cpp:

-#if JS_HAS_DESTRUCTURING
-#include "jsdhash.h"
-#endif

--------

After applying the patch, the following two warnings appear during the build:

../jsobj.h:753:30: warning: inline function 'js::GlobalObject* JSObject::asGlobal()' used but never defined
../jsobj.h:1251:17: warning: inline function 'bool JSObject::isArguments()' used but never defined

The missing definitions can be found in vm/GlobalObject.h and vm/ArgumentsObject.h respectively. I tried to include vm/GlobalObject.h in jsobj.h but that resulted in >20 errors about "forward declaration of 'struct JSObject'" and that "field 'atomMap' has incomplete type". Apparently, jsval.h defines JSObject as a struct.

And why is the include statement for "jsautooplen.h" not placed below, for example, "jsapi.h"? The position of that header was after *inlines.h headers.

I assume that this patch will not be approved, since there are two new warnings and some uncertainties about the removal of include statements. I think it is better to split this patch into two parts (approved and needs-some-work) and commit them separately, because imho reading a unified diff of 5000 lines can be confusing. Is that possible?

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 553083 [details] [diff] [review]
Applied iwyu on js/src (repository mozilla-inbound)

Reassigning the review request to Waldo, because he (a) has been working with #includes lately, and (b) is good at this nitpicky kind of stuff :)

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 553083 [details] [diff] [review]
Applied iwyu on js/src (repository mozilla-inbound)

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

So, um.  Three things.

First, I have no idea what exactly trying to accomplish here.  Getting rid of useless includes is certainly an unmitigated good.  Adding in the bootlegged ones, I don't actually know.  Aesthetically maybe it's nicer.  But it does make the list larger, possibly.  I don't know.  Anyway: I am not a particularly good person to review this.

Second, whatever we're doing here, I'm not sure whether we want to preserve existing style at all here.  That is: all <> includes first, then a blank line, then all mfbt/*.h includes, then a blank line, then all js*.h includes, then a blank line, then all vm/*.h includes, then a blank line, then all js*inlines.h includes, then a blank line, then all vm/*-inl.h includes.

Third, I'm leaving on vacation tomorrow for a couple weeks, so I am not the best person to ask for a review here if you want this handled with any promptness.  (This isn't ordinarily a problem -- feel free to ask me for a review any time after September 4 and I'll be more than happy to do it.  Just to be clear.)

So.  Um.  How about I hand this off to the module owner, and he can decide what we should do here.  :-)

Comment 18

[Nicholas Nethercote [inactive]]

FWIW, Brendan has expressed approval in the past of not bootlegging (in bug 558723, IIRC).  I agree it's something of an aesthetic thing.

On a more practical note:  accurate #include lists can make for shorter compile times, because we don't do unnecessary compilations due to bogus dependencies.  And the only way to get accurate #include lists is to use a tool like IWYU.  And if using IWYU results in longer #include lists due to less bootlegging, that seems worth it to me.

Comment 19

[Brendan Eich [:brendan]]

IWYU is awesome. njn represents my thoughts accurately, plus: faster compiles. dmandelin can be decider.

/be

Comment 20

[David Mandelin [:dmandelin]]

Comment on attachment 553083 [details] [diff] [review]
Applied iwyu on js/src (repository mozilla-inbound)

So what is the compile time improvement? I was about to test it but the patch doesn't apply to today's m-i. Could you either rebase it or just tell me what rev of m-i it applies to cleanly?

Comment 21

[David Mandelin [:dmandelin]]

Comment on attachment 553083 [details] [diff] [review]
Applied iwyu on js/src (repository mozilla-inbound)

This doesn't build on Windows. The problem is that MSVC doesn't have stdint.h--there may be others, though. See this code in jsstdint.h:

/* If we have a working stdint.h, then jsinttypes.h has already
   defined the standard integer types.  Otherwise, define the standard
   names in terms of the 'JS' types.  */
#if ! defined(JS_HAVE_STDINT_H) && \
    ! defined(JS_SYS_TYPES_H_DEFINES_EXACT_SIZE_TYPES)

typedef JSInt8  int8_t;
typedef JSInt16 int16_t;
typedef JSInt32 int32_t;
typedef JSInt64 int64_t;

typedef JSUint8  uint8_t;
typedef JSUint16 uint16_t;
typedef JSUint32 uint32_t;
typedef JSUint64 uint64_t;

I think there are a lot of ways to solve this: include jsstdint.h instead, move these declarations, etc.

Comment 23

[Ed Morley [:emorley]]

sandervv, are you still working on this? :-)

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Bug 772807 is the equivalent for editor/.

Comment 25

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove unnecessary headers (as found by include-what-you-use) from js/src/jsfoo.cpp files.

I tried IWYU for myself.  It's not perfect -- sometimes it's just wrong, and it
(unsurprisingly) doesn't know about SpiderMonkey-specific things, like that we
use mozilla/StandardInteger.h instead of stdint.h.  You can use pragmas to
teach it such things, though I had mixed success with them.

For each file, it tells you the #includes you can remove, and also the
#includes you should add.  The full #include list avoids all bootlegging (i.e.
pulling in a definition indirectly).  It turns out we have a *lot* of
bootlegging occurring in SpiderMonkey, which isn't surprising seeing that it's
difficult to avoid.

But bootlegging isn't really a problem.  My motivation was to reduce compile
times, because I've noticed SM compiles getting a lot slower recently.  

The attached patch makes two basic changes.

- It removes all the #includes in js/src/jsfoo.cpp files that IWYU told me I
  could remove;  there were over 200.  In a few places I had to add a different
  #include to avoid problems or do other tweaks.  

- In every jsfoo.cpp file, it moves the |#include "jsfoo.h"| to the very top.
  The rationale is that this ensures that every jsfoo.h can be #included
  without needing anything else #included before it.  I found several jsfoo.h
  files where that wasn't the case.  I think this should be a standard thing.

I measured from-scratch build of three versions of SM: r120000 (from late
January), r129511 (my local tip), and r129511 with my patch applied:

- d64-120000: real 1m18.882s  user 6m36.893s  sys 0m18.429s
- d64-tip:    real 1m52.934s  user 8m38.232s  sys 0m23.177s
- d64-iwyu:   real 1m38.067s  user 8m32.764s  sys 0m22.609s

This shows that I was right about SM compiles getting slower.  It also shows
that removing unnecessary #includes can make a difference (-12.5%), which is
very pleasing.

I plan to do this for the rest of SpiderMonkey (e.g. all the files within 
sub-directories of js/src/) but I'm going on vacation for a month at the end of
this week, so I thought I'd go with this chunk.  I've compiled with a number of
configurations, but I haven't tried the try server yet.

Luke, as module owner, I figure you get this review :)

Comment 26

[Andrew McCreight [:mccr8]]

Pretty cool.  I'll have to try it on some DOM code.  It might even be worth doing it for our autogenerated bindings code.

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 741186 [details] [diff] [review]
(part 1) - Remove unnecessary headers (as found by include-what-you-use) from js/src/jsfoo.cpp files.

Cool!  I like faster compile times and less #includes.  I also like the "#include your own .h first to make sure it compiles on its own" rule and I've used that in personal projects.  Could you send a mail to dev-tech-js-engine-internals announcing this official addendum to the #include style guide? (#include style seems to have been in the lossage due to bug 861017...)

Comment 28

[Nicholas Nethercote [inactive]]

> Could you send a mail to
> dev-tech-js-engine-internals announcing this official addendum to the
> #include style guide?

Done.

I got some bustage on Android and Mac 10.7 opt builds on try server, but they shouldn't be hard to fix.

Comment 29

[Nicholas Nethercote [inactive]]

Alas, the 15 second speed-up was too good to be true.  I measured again more carefully and got maybe a 1--2 second speed-up on my Linux desktop machine (which has a magnetic disk) and a negiglible change on my Mac laptop (which has an SSD).

I did an experiment where I added a #warning statement to the very top of every single .c, .cpp and .h file in SpiderMonkey, and compiled from scratch.  There are 258 .cpp files, 5 .c files, and a total of 127,720(!) headers get pulled in.  Here's the top 10:

127983 counts:
( 1)   5473 ( 4.3%,  4.3%): #warning ./jscntxt.h
( 2)   4609 ( 3.6%,  7.9%): #warning ./jsapi.h
( 3)   4318 ( 3.4%, 11.3%): #warning ./jsobj.h
( 4)   3867 ( 3.0%, 14.3%): #warning ./jsprvtd.h
( 5)   3839 ( 3.0%, 17.3%): #warning ./jspubtd.h
( 6)   3576 ( 2.8%, 20.1%): #warning ./jstypes.h
( 7)   3519 ( 2.7%, 22.8%): #warning ./gc/Barrier.h
( 8)   3065 ( 2.4%, 25.2%): #warning ./jsutil.h
( 9)   2745 ( 2.1%, 27.4%): #warning ./assembler/wtf/Platform.h
(10)   2721 ( 2.1%, 29.5%): #warning ./jscompartment.h

I.e. jscntxt.h gets pulled in 5473, which is more than 20 times per file on average.  What a busted module system C and C++ have.

The changes in my patch only reduced that to 125,806.  But there's hope yet.  My patch only modifies the #include statements within .cpp files, but because the #include hierarchy is so dense -- #include any header and you'll invariable end up pulling in most of the others -- I'll need to trim the #include statements within .h files to have a decent effect.

I haven't done that yet because IWYU wasn't giving me any info on most .h files.  But I looked more closely at the output and saw that lots of static assertions were failing in the IWYU builds (not sure why) so I disabled them and now it's giving me info on .h files.  I'll work on them and hopefully that'll have a bigger effect.

I still plan to land the changes in the attached patch (modulo a handful of changes to fix various bits of minor bustage) because it still seems virtuous and it'll likely rot very quickly.

Comment 30

[Nicholas Nethercote [inactive]]

Attachment: script to add #warning to the top of every file

This is the script I used for the "add #warning to the top of every file in SpiderMonkey" experiment, in case anyone is interested.  (BTW, when you compile, pipe the output to a file, and make sure you build with -j1.)

Comment 31

[Nicholas Nethercote [inactive]]

Part 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ab1119d4596

Comment 32

[Nicholas Nethercote [inactive]]

> There are 258 .cpp files, 5 .c files, and a total of 127,720(!)
> headers get pulled in.  Here's the top 10:

After doing all the .h files, I've got that down to 103,067, reducing compile time on my Linux64 desktop from ~99s to ~95s.

When I get back from vacation I'll graph the header dependencies (either using http://www.flourish.org/cinclude2dot/ or just writing my own script) and see if anything obvious pops out.

Comment 33

[Luke Wagner [:luke]]

First of all, I spend no small amount of time waiting on builds, as do many JS engine devs, so I appreciate this line of investigation!

(In reply to Nicholas Nethercote [:njn] (away until June 3) from comment #29)
> What a busted module system C and C++ have.

They ain't got one!  But maybe one day they will: https://www.youtube.com/watch?v=4Xo9iH5VLQ0.

(In reply to Nicholas Nethercote [:njn] (away until June 3) from comment #32)
> When I get back from vacation I'll graph the header dependencies (either
> using http://www.flourish.org/cinclude2dot/ or just writing my own script)
> and see if anything obvious pops out.

I've thought a bit about this before and, FWIW, the following rough idea appeals to me:
 - Accept that there are a set of .h files that are going to be #included basically everywhere.
 - Make some new header, js.h, or whatever, that #includes this common set and we #include "js.h" everywhere.  Focus on shrinking the size of js.h (and its transitive #includes) to the minimum definitions required. Once we have js.h in a good state, we lock it down so that it stays that way.  Possible ways of doing this include:
  + put all the headers transitively #included by js.h go in a new dir js/private
  + have make check that all non-system headers transitively #included by js.h (using gcc -M) are in js/public or js/private
 - For all the other headers (not #included by js.h), we apply the rule: we shouldn't need to #include this header in more than a handeful of .cpps.  Analyses can point us to top offenders.

Comment 34

[Nicholas Nethercote [inactive]]

>  - Accept that there are a set of .h files that are going to be #included
> basically everywhere.

Yeah... I'm worried that the amount of "include everywhere" code is going to be *large*.  Look at comment 29 -- jscntxt.h, jsapi.h, and jsobj.h are included the most, and they're not small.

(Looking closely, I'm perplexed by the fact that jscntxt.h was included more times than jsapi.h even though jscntxt.h includes jsapi.h itself.  Hmm.)

Anyway, I'm reserving judgment about these things until I get back and can generate the dependency graph.  Hopefully that will reveal a lot.

Comment 35

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] (away until June 3) from comment #34)
> Yeah... I'm worried that the amount of "include everywhere" code is going to
> be *large*.  Look at comment 29 -- jscntxt.h, jsapi.h, and jsobj.h are
> included the most, and they're not small.

True, but I was thinking, for this "included everywhere" set, we'd try to break apart headers that contain unnecessary junk.

One inherent problem is that our hands are bound by "everyone needs the definition of JSContext" and "JSContext needs the definition of 100 different things because they are members of JSContext".  We could break this dependency chain (and probably free of cost for compilers with LTO):

// jscntxt.h

class FooData;  // forward declaration; don't have to #include "Foo.h"

class JSContext {
  FooContext &fooContext();
}

// jscntxt.cpp

#include "Foo.h"

struct ContextImpl : JSContext {
  FooData fooData;
};

FooData &JSContext::fooContext() {
  return static_cast<ContextImpl*>(this)->fooData;
}

// Foo.h

class FooData {
  .. stuff Foo.cpp wants
}

// Foo.cpp

#include "Foo.h"
#include "jscntxt.h"

void foo(JSContext *cx) {
  cx->fooContext().stuff ...
}

Of course, maybe JSContext isn't the major problem.  It'd be nice to have a ranking of not just how often files are included, but how many bytes they contribute.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ab1119d4596

Comment 37

[Norbert Lindenberg]

The patch above broke the Internationalization API, which is still not enabled by default and therefore easy to break:

/Users/standards/mozilla/intl/js/src/jsapi.cpp:1849:6: error: use of undeclared identifier 'js_InitIntlClass'
    {js_InitIntlClass,                  EAGER_ATOM_AND_CLASP(Intl)},
     ^
1 error generated.

Comment 38

[Norbert Lindenberg]

Attachment: Fix build with Internationalization API

Comment 39

[Nicholas Nethercote [inactive]]

Comment on attachment 742662 [details] [diff] [review]
Fix build with Internationalization API

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

Stealing review.  I can't do the check-in, though, because I'm about to go on vacation.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab9fb5ce21d7

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ab9fb5ce21d7

Comment 42

[Nicholas Nethercote [inactive]]

Oh, this is interesting.  AFAICT, if you have Foo.cpp, IWYU will analyze header files called Foo.h and Foo-inl.h.  But you have jsfoo.cpp, IWYU doesn't seem to look at jsfooinlines.h.

I could be wrong about this, but I'm not getting any info on jsfooinlines.h headers, whereas I am getting info about various Foo-inl.h headers (and Foo.h headers), and that info always precedes the info for Foo.cpp.

Comment 43

[Nicholas Nethercote [inactive]]

Attachment: part 2

This patch addresses part of what IWYU finds.  (But there's plenty more to come
in subsequent patches.)  It applies on top of patches from bug 879831 and bug
880565.

It reduces the #included file count from 113,780 to 111,121, and the #included
byte count from 3.01 GiB to 2.93 GiB.

I've attached a graph showing the resulting #include dependencies at
www.valgrind.org/njn/headers.png.  The graph shows just the .h files, and it
excludes files from ion/, yarr/ and assembler/ to keep it a reasonable size.
The most obvious thing from the graph is that jsobj.h and jscntxt.h are
critical.  (I have plans to pull out JSRuntime from jscntxt.h.)

Comment 44

[Nicholas Nethercote [inactive]]

> I've attached a graph showing the resulting #include dependencies at
> www.valgrind.org/njn/headers.png.

BTW, viewing it in a browser is hopeless.  You'll need to use a PNG viewer that gives fine-grained control over the zoom level.  I've been using Preview on Mac.

Comment 45

[Nicholas Nethercote [inactive]]

Attachment: script to generate #include graph

Here's the script I used to produce the graph, BTW.  It writes a 'dot' format file to stdout.  Use 'dot' (part of GraphViz) to convert it to PNG, PostScript, or another format.  E.g. for PNG:

  dot -Tpng x.dot -o x.png

Comment 46

[Nicholas Nethercote [inactive]]

Attachment: part 2a and 2b

More from IWYU.

Comment 47

[Jason Orendorff [:jorendorff]]

Comment on attachment 759646 [details] [diff] [review]
part 2

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

Looks good.

::: js/src/frontend/BytecodeCompiler.h
@@ +7,5 @@
>  #ifndef BytecodeCompiler_h__
>  #define BytecodeCompiler_h__
>  
>  #include "jsapi.h"
> +#include "jsscript.h"

Whoa, this doesn't need jsscript.h.

If the IWYU tool suggests including struct/class/enum definitions when we only need declarations, that's going to overinclude on headers like this that are actually nicely isolated. Maybe worth fixing in IWYU upstream!

In fact this header doesn't even need jsapi.h, though removing it would be no great savings...

::: js/src/vm/ArgumentsObject.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 "vm/ArgumentsObject.h"

This is fine, but in passing, I think we should change the rule so that if there's an -inl header, *that* should be included first, and it should include the non-inl header first. (Given the intent of the rule, this seems only logical.)

::: js/src/vm/ArgumentsObject.h
@@ +8,5 @@
>  #define ArgumentsObject_h___
>  
>  #include "jsfun.h"
>  
> +#include "vm/Stack.h"

This include seems unnecessary.

Comment 48

[Nicholas Nethercote [inactive]]

> > +#include "jsscript.h"
> 
> Whoa, this doesn't need jsscript.h.
> 
> If the IWYU tool suggests including struct/class/enum definitions when we
> only need declarations, that's going to overinclude on headers like this
> that are actually nicely isolated. Maybe worth fixing in IWYU upstream!

Good catch.  IWYU gets this right in general, and in this case it recommended forward-declaring JSScript.  I was just lax in reading the output carefully.

Comment 49

[Nicholas Nethercote [inactive]]

Part 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbca87e3009

Comment 50

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4bbca87e3009

Comment 51

[Jason Orendorff [:jorendorff]]

Ooh, I really don't like this change:

   15.12 -#include "jsprvtd.h"
   15.13 +
   15.14 +class JSLinearString;
   15.15  
   15.16  namespace js {
   15.17 +
   15.18 +class AutoNameVector;
   15.19 +class LazyScript;
   15.20 +struct SourceCompressionToken;
   15.21 +

jsprvtd.h of course contains those forward declarations. It seems more straightforward and less error prone to have one header full of forward declarations of types than to manually put in forward declarations in every header that mentions a class.

Looking at part 3 now...

Comment 52

[Nicholas Nethercote [inactive]]

Can you explain exactly what jspubtd.h and jsprvtd.h are for?  I think partly they contain exactly these kinds of forward declarations, with jspubtd.h (presumably) holding those that are necessary for external API consumers.

But both files also contain a bunch of other stuff -- some struct declarations, some enums, some #define constants, some other typedefs.  There are comments at the tops of the files but I still don't feel like I understand what should be there and what should not.  If we take your comment to its logical conclusion there shouldn't be *any* forward class or struct declarations anywhere else in the code, right?

Comment 53

[Jeff Walden [:Waldo]]

jsprvtd.h and jspubtd.h also mean that any time someone touches those files, you basically get a clobber.  Forward-declaring only in the files that name individual types eliminates that.  That's a pretty huge win in my book.

Comment 54

[Nicholas Nethercote [inactive]]

Yeah, I'm not a fan of having headers that have to be #included everywhere unless they're really very small and contain only truly essential stuff.

Comment 55

[Jason Orendorff [:jorendorff]]

Comment on attachment 760772 [details] [diff] [review]
part 2a and 2b

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

::: js/src/jsapi.h
@@ -24,5 @@
>  #include "jsalloc.h"
>  #include "jspubtd.h"
> -#include "jsutil.h"
> -
> -#include "js/Anchor.h"

Careful removing stuff from jsapi.h, it's pretty widely used (even in third-party code outside our tree). Please at least put these in a separate changeset, and Try-server them thoroughly; and if you decide you're too timid to change this header after all, that's fine with me!

::: js/src/jscntxt.h
@@ +123,4 @@
>  }
>  
> +class AsmJSActivation;
> +class InterpreterFrames;

I'd prefer using jsprvtd.h here.

::: js/src/jsdhash.h
@@ +9,5 @@
>  
>  /*
>   * Double hashing, a la Knuth 6.
>   *
>   * Try to keep this file in sync with xpcom/glue/pldhash.h.

It seems to be a little out of sync already. Take a look?

::: js/src/jsgc.h
@@ +12,4 @@
>  #include "mozilla/DebugOnly.h"
>  #include "mozilla/Util.h"
>  
> +#include <setjmp.h>

I can't tell what rules IWYU is using. This header is only needed in jscntxt.h, which uses jmp_buf... I think.

::: js/src/vm/RegExpStatics.h
@@ +13,2 @@
>  
> +class JSObject;

Another forward declaration.

::: js/src/vm/Xdr.cpp
@@ +12,2 @@
>  
>  #include "vm/Xdr.h"

#include "vm/Xdr.h" should go first.

::: js/src/yarr/YarrCanonicalizeUCS2.cpp
@@ +27,2 @@
>  
>  // DO NOT EDIT! - this file autogenerated by YarrCanonicalizeUCS2.js

The file seems to be autogenerated!

Comment 56

[Nicholas Nethercote [inactive]]

> Careful removing stuff from jsapi.h

Ok, I'll put those changes in a separate patch.


>> +class AsmJSActivation;
>> +class InterpreterFrames;
>
> I'd prefer using jsprvtd.h here.

Those aren't declared in jsprvtd.h, and that file already has numerous other forward declarations of this sort.  I'll leave this as is and if we want to make the js{prv,pub}td.h story more consistent, either by using them everywhere or removing them, that can be a follow-up.


> >   * Try to keep this file in sync with xpcom/glue/pldhash.h.
> 
> It seems to be a little out of sync already. Take a look?

Trying to get the #includes to match is fruitless.  And we don't seem to have any users of JSDHash anyway... I'll see if it can be removed.


> > +#include <setjmp.h>
> 
> I can't tell what rules IWYU is using. This header is only needed in
> jscntxt.h, which uses jmp_buf... I think.

True.  IWYU is just flat-out wrong about 10% of the time.  Usually when it's wrong it claims you don't need a header when you actually do, but I guess this is an exception.


> > +class JSObject;
> 
> Another forward declaration.

I replaced that one with #include jspubtd.h.


> #include "vm/Xdr.h" should go first.

Done.


> ::: js/src/yarr/YarrCanonicalizeUCS2.cpp
> @@ +27,2 @@
> >  
> >  // DO NOT EDIT! - this file autogenerated by YarrCanonicalizeUCS2.js
> 
> The file seems to be autogenerated!

And yet we have it checked in.  Yuk.  I think the least worst path forward is to modify both the file and the generation script so they are consistent (they currently aren't).  I'll move the #includes out of the JSC::Yarr namespace, too(!)

Comment 57

[Nicholas Nethercote [inactive]]

> we don't seem to have any users of JSDHash anyway... I'll see if it can be removed.

See bug 884649.

Comment 58

[Nicholas Nethercote [inactive]]

Parts 2a and 2b:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4558994f91a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c148efceaf9

Comment 59

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4558994f91a0
https://hg.mozilla.org/mozilla-central/rev/7c148efceaf9

Comment 60

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove more unnecessary headers (as found by include-what-you-use) from SpiderMonkey.

Updated and rebased.  Hooray.

Comment 61

[Nicholas Nethercote [inactive]]

Comment on attachment 765213 [details] [diff] [review]
(part 3) - Remove more unnecessary headers (as found by include-what-you-use) from SpiderMonkey.

Oh, the patch that I called "patch 3" when I uploaded I accidentally landed in two parts as "part 2a and 2b".  So I've called this "patch 3" as well... but note that it's different to the previous "patch 3" patch.  Sorry.

Comment 62

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

njn, please test your patch(es) against deterministic builds. Already there's been an instance of bustage in bug 866916 and now:

https://hg.mozilla.org/mozilla-central/rev/7c148efceaf9#l47.19

has caused the same bustage again, in bug 885502.

Unfortunately, it wouldn't be an optimal use of time to keep doing this to and fro everything, so please whitelist that "#include "ds/Sort.h"" line, thanks.

Comment 63

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(I also overheard on IRC that this affects ggc too, and it's not just a single line of include)

Comment 64

[Jason Orendorff [:jorendorff]]

(In reply to Nicholas Nethercote [:njn] from comment #52)
> Can you explain exactly what jspubtd.h and jsprvtd.h are for?  I think
> partly they contain exactly these kinds of forward declarations, with
> jspubtd.h (presumably) holding those that are necessary for external API
> consumers.

They should only have typedefs and forward declarations in them. Enums are OK I guess. The file should be lightweight to include; I don't think it should #include HashTable or Vector and in fact I bet you can

-#include "js/HashTable.h"
-#include "js/Vector.h"
-
-/*
- * Convenience constants.
- */
-#define JS_BITS_PER_UINT32_LOG2 5
-#define JS_BITS_PER_UINT32      32
-
-/* The alignment required of objects stored in GC arenas. */
-static const unsigned JS_GCTHING_ALIGN = 8;
-static const unsigned JS_GCTHING_ZEROBITS = 3;

without breaking anything.

> But both files also contain a bunch of other stuff -- some struct
> declarations, some enums, some #define constants, some other typedefs. 
> There are comments at the tops of the files but I still don't feel like I
> understand what should be there and what should not. 

The comment in jsprvtd.h is probably a decade old. I vote for replacing that with a one-liner.

> If we take your
> comment to its logical conclusion there shouldn't be *any* forward class or
> struct declarations anywhere else in the code, right?

Right. We could forward-declare instead, maybe delete jsprvtd.h entirely -- but let's do that only if you think the forward declarations communicate something useful. If you share my opinion that it's just boilerplate, by all means let's store all that in a single closet.

Comment 65

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #53)
> jsprvtd.h and jspubtd.h also mean that any time someone touches those files,
> you basically get a clobber.  Forward-declaring only in the files that name
> individual types eliminates that.  That's a pretty huge win in my book.

Well, nothing is going to make your incremental builds fast when you pull a change to jsapi.h or any of the dozen or more files it includes -- i.e. just about every time you pull. jspubtd.h is one of those files; there is no penalty for using it that you're not already paying.

I'm interested in reducing the #include load because it can make incremental builds faster when you've edited a small number of files locally. Pulls will always cost you; I don't think it can be fixed.

Comment 66

[Jason Orendorff [:jorendorff]]

Comment on attachment 765213 [details] [diff] [review]
(part 3) - Remove more unnecessary headers (as found by include-what-you-use) from SpiderMonkey.

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

Looks good, r=me.

::: js/src/jsutil.h
@@ +6,4 @@
>  
>  /*
>   * PR assertion checker.
>   */

Can you kill the bizarre comment while you're in here?

Comment 67

[Nicholas Nethercote [inactive]]

Comment on attachment 765213 [details] [diff] [review]
(part 3) - Remove more unnecessary headers (as found by include-what-you-use) from SpiderMonkey.

Bug 886205 and others have rendered part 3 out of date.  I'm going to close this bug and finish the IWYU work in bug 888768.