Closed Bug 905017 Opened 6 years ago Closed 6 years ago

Minimize #includes of JS engine headers in the browser

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: njn, Assigned: njn)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [js:t])

Attachments

(4 files, 7 obsolete files)

116.84 KB, text/plain
Details
47.39 KB, patch
billm
: review+
Details | Diff | Splinter Review
19.36 KB, patch
billm
: review+
Details | Diff | Splinter Review
115.06 KB, patch
njn
: review+
Details | Diff | Splinter Review
billm suggested that lots of |#include "jsapi.h"| statements in the browser aren't necessary, and can be removed and/or replaced with e.g. |#include "jspubtd.h"|.  These are worth tackling because jsapi.h is included in lots of places, and is modified frequently (see bug 901132).

While I'm doing this, I'll look for other unnecessary includes of JS engine headers.
Component: JavaScript Engine → General
There are some .idl files which include jsapi.h in order to get the jsval name (JSContext is also commonly needed, but that can be forward-declared.)  Would it be possible to include js/Value.h instead for that purpose?
header.py automatically includes jspubtd.h if you use jsval/implicit_jscontext, so these includes can probably be dropped right away.
(In reply to comment #2)
> header.py automatically includes jspubtd.h if you use jsval/implicit_jscontext,
> so these includes can probably be dropped right away.

Oh, I was under the impression that for jsval you need jsapi.h, not jspubtd.h...
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> Oh, I was under the impression that for jsval you need jsapi.h, not
> jspubtd.h...

You only need js/Value.h.
The common cases where jsapi.h can be removed:

- For JS::Value or jsval you need js/Value.h.
- For various kinds of Handle you need js/RootingAPI.h
- For pointers to JSContext, JSObject, and some others, you need jspubtd.h.

I've also seen numerous cases where jsapi.h is needed, but only for, say, one function or type.  I'm thinking about breaking some small parts out of jsapi.h into separate files (as we've already started in js/public/) to further minimize Gecko includes of jsapi.h, but if I do that it'll be as a follow-up.
(In reply to Nicholas Nethercote [:njn] from comment #5)
> - For pointers to JSContext, JSObject, and some others, you need jspubtd.h.

For pointers, without dereference (which is common in headers themselves), you don't need an include at all. You need a forward declaration.
> For pointers, without dereference (which is common in headers themselves),
> you don't need an include at all. You need a forward declaration.

I've been including jspubtd.h instead because it seemed simpler, and achieved the primary goal of not including jsapi.h.  But I can do forward decls instead if there are strong opinions.
(In reply to Nicholas Nethercote [:njn] from comment #7)
> I've been including jspubtd.h instead because it seemed simpler, and
> achieved the primary goal of not including jsapi.h.  But I can do forward
> decls instead if there are strong opinions.

I think we should, in headers, avoid including other headers as much as possible.
To ensure maximal #include removal, I did this by first (with a script)
commenting out every #include statement (outside of js/src/) that included a JS
header, and then fixing all the subsequent bustage.  This may have caused a
fair amount of bootlegging, but I'm not worried about that;  it happens all the
time anyway.

As usual, because of pre-existing bootlegging, the removal of some #includes
necessitates the addition of others.  But not that many in this patch.  jsprf.h
is the most notable one;  I removed it from xpcpublic.h, which meant I had to
add it to a bunch of xpconnect .cpp files.

jsfriendapi.h, jsdbgapi.h and jswrapper.h all include jsapi.h.  So in any file
that included one of those three, I removed a |#include jsapi.h| if present.
This increases bootlegging, but that doesn't worry me, and it made things
easier for the patch.

I looked at the following three files that generate code that contains jsapi.h
includes:

- dom/bindings/Configuration.py:  Unchanged.  It #includes jsapi.h if JSObject
  is somehow involved.  I don't entirely understand it, and the jsapi.h include
  might be replaceable at least some of the time with smaller headers and/or
  forward decls, but I didn't do try to do that.

- js/xpconnect/src/dictionary_helper_gen.py:  I was able to replace this
  include with some forward decls.

- js/xpconnect/src/qsgen.py: Unchanged, because it genuinely needs jsapi.h.

Here are the removed #includes:

    175 -#include "jsapi.h"
     75 -#include "jsfriendapi.h"
     16 -#include "jspubtd.h"
     13 -#include "jsdbgapi.h"
      7 -#include "jsproxy.h"
      6 -#include "js/HashTable.h"
      5 -#include "js/RootingAPI.h"
      4 -#include "jswrapper.h"
      3 -#include "jstypes.h"
      2 -#include "js/Value.h"
      2 -#include "js/Utility.h"
      2 -#include "jsprf.h"
      2 -#include "jsclass.h"
      2 -#include "js/HeapAPI.h"
      1 -#include "prprf.h"
      1 -#include "js/Vector.h"
      1 -#include "js/GCAPI.h"
    317 total

Here are the added #includes:

     15 +#include "jsprf.h"
      6 +#include "jsdbgapi.h"
      4 +#include "js/Value.h"
      3 +#include "jswrapper.h"
      2 +#include "jsapi.h"
      2 +#include "js/RootingAPI.h"
      1 +#include <stdint.h>
      1 +#include <stddef.h>
      1 +#include "mozilla/Assertions.h"
      1 +#include "jspubtd.h"
      1 +#include "jsfriendapi.h"
      1 +#include "jsclass.h"
     38 total

Here are the added forward decls:

      4 +class JSObject;
      3 +struct JSContext;
      3 +template <typename T> class MutableHandle;
      3 +template <typename T> class Handle;
      3 +class Value;
      1 +class CallArgs;
      1 +class AutoIdVector;
     18 total

The total changes are -279 #include statements, +18 forward decls.

Next, I will time how long it takes to rebuild the browser after touching
jsapi.h.

I haven't try-server'd yet, and there will undoubtedly be some bustage, but I
don't expect the numbers to change significantly.
Attachment #790642 - Flags: review?(wmccloskey)
The rebuild-after-touching-jsapi.h results were disappointing.  I did three debug runs of each on my linux64 box, and am showing the fastest one for each.

Before:

real  15m27.767s
user  95m50.724s
sys    4m28.216s


After:

real  15m12.082s
user  94m31.476s
sys    4m25.724s


I know there's a bunch of stuff that's constant in both cases -- rebuilding the JS engine, linking libxul, plenty of other stuff... but still, that's less than 2% improvement.  Hmm.  Tomorrow I'll take a close look at the build output to see what's taking up the time.
mozilla/dom/BindingUtils.h and js/xpconnect/src/xpcprivate.h both still depend on jsapi.h, and those headers are included all over the place...
> mozilla/dom/BindingUtils.h and js/xpconnect/src/xpcprivate.h both still
> depend on jsapi.h, and those headers are included all over the place...

And breaking those dependencies is *really* hard.  Sigh.
(In reply to comment #12)
> > mozilla/dom/BindingUtils.h and js/xpconnect/src/xpcprivate.h both still
> > depend on jsapi.h, and those headers are included all over the place...
> 
> And breaking those dependencies is *really* hard.  Sigh.

But perhaps we can tackle this problem in pieces?
Comment on attachment 790642 [details] [diff] [review]
Minimize #includes of JS engine headers in the browser.

I'm worried that this patch will take us in the wrong direction. My main concern is that it removes a lot of jsapi.h includes from .cpp files. It seems likely that removing that code only works because some other included header already includes jsapi.h. But the goal of this bug is to stop including jsapi.h from other headers. Once we do that, we're probably just going to have to revert the changes to a lot of these .cpp files.

For now, I think it's better to concentrate on jsapi.h inclusions in header files. That's where almost all the benefits are. Once we've done that, we can fix the .cpp files if we still feel like it.

Can you make a patch that doesn't remove inclusions from .cpp files?
> it removes a lot of jsapi.h includes from .cpp files. It
> seems likely that removing that code only works because some other included
> header already includes jsapi.h. 

If you replace ".cpp" with ".h" in that first sentence, the entire statement is still true.

So basically, you're worried about increasing bootlegging.

> But the goal of this bug is to stop
> including jsapi.h from other headers.

The real goal is to stop including jsapi.h into .cpp files.  Removing includes from headers is a big part of that, but it's not the whole story.
I went through the first ten or so .cpp files. A few of them don't seem to use the JS API (based on grepping for js:: and js_ case insensitively). Some only use JS::Handle, so we could probably get away with just RootingAPI.h. But at least half actually call some function defined in jsapi.h or jsfriendapi.h. If we remove those includes now, we're just going to have to put them back later.

I guess what I'm saying is: it's fine to do an automated change as long as it mostly achieves the effect we want. But when it's wrong over half the time, I don't think it makes sense to do.
I did some analysis with CHECK_REBUILD=1 after touching jsapi.h, xpcprivate.h and BindingUtils.h.  Here are the most common causes for rebuilt files:

2930 counts:
( 1)   1718 (58.6%, 58.6%): because jsapi.h changed
( 2)    783 (26.7%, 85.4%): because jsapi.h, BindingUtils.h changed
( 3)     65 ( 2.2%, 87.6%): because BindingUtils.h, jsapi.h changed
( 4)     37 ( 1.3%, 88.8%): because libxpcomglue_s.a, libjs_static.a changed
( 5)     18 ( 0.6%, 89.5%): because xpcprivate.h, jsapi.h changed
( 6)     14 ( 0.5%, 89.9%): because libxpcomglue_s.a changed
( 7)     13 ( 0.4%, 90.4%): because jsapi.h, BindingUtils.h, xpcprivate.h changed
( 8)     12 ( 0.4%, 90.8%): because xpcprivate.h, jsapi.h, BindingUtils.h changed
( 9)     10 ( 0.3%, 91.1%): because jsapi.h, xpcprivate.h changed
(10)      8 ( 0.3%, 91.4%): because jsapi.h, xpcprivate.h, BindingUtils.h changed

So:
- xpcprivate.h is a red herring;  it's only involved in about 2% of the rebuilt files.
- BindingUtils.h is important, involved in ~30% of rebuilt files.
- There's still ~60% of rebuild that don't involve BindingUtils.h or xpcprivate.h.  So I guess there's one or more oft-included headers that still include jsapi.h.
A lesson I keep forgetting:  header includes are a power-law-ish network (i.e. a few headers are important, many headers aren't important), and when you want to significantly break up such a network, random targetting is unlikely to have an effect -- you need to target the important nodes.

I'm part way through a browser build with -H on, which tells me exactly which headers are being included.  One case I noticed yesterday was that most of moz/storage/ was being rebuilt when jsapi.h was touched, which seemed odd.  The -H output tells me it's almost entirely due to three files:

- xpcexception.h, which is generated from xpcexception.idl, which can be made to not include jsapi.h very easily.

- IPCMessageUtils.h, which can be made to not include jsapi.h very easily.

- PseudoStack.h, which includes jsfriendapi.h (which includes jsapi.h) for a small amount of profiler stuff, which could be split out fairly easily.
(In reply to Nicholas Nethercote [:njn] from comment #17)
> I did some analysis with CHECK_REBUILD=1 after touching jsapi.h,
> xpcprivate.h and BindingUtils.h.  Here are the most common causes for
> rebuilt files:
> 
> 2930 counts:
> ( 1)   1718 (58.6%, 58.6%): because jsapi.h changed
> ( 2)    783 (26.7%, 85.4%): because jsapi.h, BindingUtils.h changed
> ( 3)     65 ( 2.2%, 87.6%): because BindingUtils.h, jsapi.h changed
> ( 4)     37 ( 1.3%, 88.8%): because libxpcomglue_s.a, libjs_static.a changed
> ( 5)     18 ( 0.6%, 89.5%): because xpcprivate.h, jsapi.h changed
> ( 6)     14 ( 0.5%, 89.9%): because libxpcomglue_s.a changed
> ( 7)     13 ( 0.4%, 90.4%): because jsapi.h, BindingUtils.h, xpcprivate.h
> changed
> ( 8)     12 ( 0.4%, 90.8%): because xpcprivate.h, jsapi.h, BindingUtils.h
> changed
> ( 9)     10 ( 0.3%, 91.1%): because jsapi.h, xpcprivate.h changed
> (10)      8 ( 0.3%, 91.4%): because jsapi.h, xpcprivate.h, BindingUtils.h
> changed
> 
> So:
> - xpcprivate.h is a red herring;  it's only involved in about 2% of the
> rebuilt files.

Right; we try pretty hard to not include it outside of xpconnect/bindings.

> - BindingUtils.h is important, involved in ~30% of rebuilt files.

This one should mostly be limited to bindings code as well, but isn't at this point. I'm working on that (bug 905747, bug 905751).

> - There's still ~60% of rebuild that don't involve BindingUtils.h or
> xpcprivate.h.  So I guess there's one or more oft-included headers that
> still include jsapi.h.

Probably BindingDeclarations.h. On first sight, it doesn't looks like it uses anything from jsapi.h.
> Probably BindingDeclarations.h. On first sight, it doesn't looks like it
> uses anything from jsapi.h.

I'll try it, but according to jcranmer's analysis in bug 901132 that #include statement's criticality is a mere 2.
(In reply to Nicholas Nethercote [:njn] from comment #20)
> > Probably BindingDeclarations.h. On first sight, it doesn't looks like it
> > uses anything from jsapi.h.
> 
> I'll try it, but according to jcranmer's analysis in bug 901132 that
> #include statement's criticality is a mere 2.

3434 source files transitively include jsapi.h and 2664 source files transitively include BindingDeclarations.h. Since BindingDeclarations.h includes jsapi.h, there's no way we can get rid of 2664 of the 3434 dependencies without fixing BindingDeclarations.h. So we need to fix it.

Presumably its criticality is low because, in many source files, fixing BindingDeclarations.h is not sufficient to completely eliminate the jsapi.h dependency. But that just means we'll have to fix other stuff as well in order to make progress.
> Since BindingDeclarations.h
> includes jsapi.h, there's no way we can get rid of 2664 of the 3434
> dependencies without fixing BindingDeclarations.h. So we need to fix it.

True.  Some other files that are on my hit list that should be easy:

- GeckoProfiler{,Impl,Func}.h
- nsIScriptContext.h
- Workers.h
- nsEventListenerManager.h
- nsIDOMFile.idl

BindingUtils.h is also a big one, but it actually uses a lot of JSAPI stuff and so will be much harder.

I've already fixed a few (xpcexception.idl, nsIXPConnect.idl, IPCMessageUtils.h) and it hardly made a dent, because apparently most of the files that include jsapi.h do it in multiple ways.  Hopefully after doing the above ones I'll start to see some impact.
Attachment #790642 - Attachment is obsolete: true
Attachment #790642 - Flags: review?(wmccloskey)
> 3434 source files transitively include jsapi.h and 2664 source files
> transitively include BindingDeclarations.h.

BTW, how did you compute that?  I don't suppose you have a list of files that transitvely include jsapi.h, ordered by how many times each one is transitively included in the codebase?
There's a list in bug 901132.
This is the same as the list posted in bug 901132 (i.e., a list of headers ordered by how many .cpp files include them transitively). However, I eliminated headers that don't transitively include jsapi.h. So this is a pretty good target list, I think.
This patch fixes a few of the easy cases that are high up on the list. In particular, it eliminates:

dom/bindings/BindingDeclarations.h (ranked #4)
xpcexception.h (#8)
xpccomponents.h (#9)
nsIXPConnect.h (#10)
Attachment #791893 - Flags: review?(n.nethercote)
I had to fix a problem with GCAPI.h also.

After these two patches, the list I posted earlier looks as follows:

 2774  js/src/jsfriendapi.h
 2774  js/src/jsclass.h
 2255  js/src/jsproxy.h
 2009  js/xpconnect/src/xpcpublic.h
 2009  dom/bindings/DOMJSClass.h
 1974  dom/bindings/DOMJSProxyHandler.h
 1925  dom/base/nsJSEnvironment.h
 1888  content/events/src/Touch.h
 1834  widget/nsGUIEvent.h
 1788  objdir-ff-dbg/dom/bindings/DocumentBinding.h
 1788  content/base/public/nsIDocument.h
 1671  content/base/public/nsIContent.h
 1510  layout/base/nsIPresShell.h
 1493  layout/base/nsPresContext.h
 1440  content/base/src/nsNodeUtils.h

I think one of the biggest wins will be from decoupling jsfriendapi.h and jsapi.h. It's hard to say how difficult that will be. One thing that might be nice is to start moving stuff from jsfriendapi.h into smaller js/public include files, but in a way that makes clear that these are "friend" APIs only. Personally, I think the "friend" distinction is pretty meaningless nowadays, but other people seem to like it.
Attachment #792001 - Flags: review?(n.nethercote)
(In reply to Bill McCloskey (:billm) from comment #27)
> include files, but in a way that makes clear that these are "friend" APIs
> only. Personally, I think the "friend" distinction is pretty meaningless
> nowadays, but other people seem to like it.

I've never understood what it's supposed to mean. The only two possibilities I've come up with are "stuff that only XPConnect should ever need to use", or "stuff that accesses things that are internal enough that you have to be willing to deal with quite a bit of churn". But those are just guesses. And it's included in enough places that #1 would be a total lie.

Whatever the intention is, it really ought to be in a comment at the top of jsfriendapi.h.
I'm glad you're interested in helping.  Having said that, this bug is assigned to me, I'm working on it right now, and you're duplicating effort -- the patches in my local queue already have a lot of the changes in the patches you just posted.

On a different note, the real metric of interest for this bug is how many files get recompiled when jsapi.h is touched.  That's hard to get down because there are so many ways for files to transitively include jsapi.h.  In my current patch queue I've got it down from 2640 to 2539, i.e. not much, even though I've reduced the number of build-time transitive includes of jsapi.h from 94,396 to 49,377.   

BindingDeclarations.h, xpcpublic.h, and DOMJSClass.h are all sticking points, because they're big and have multiple dependencies on jsapi.h and friends.
Comment on attachment 791893 [details] [diff] [review]
fix a few .idl files and BindingDeclarations.h

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

::: js/src/jspubtd.h
@@ +34,5 @@
>  
>  class JS_PUBLIC_API(AutoGCRooter);
>  
>  struct Zone;
> +struct CompartmentOptions;

This is harder than it looks, because CompartmentOptions is JS_PUBLIC_API.  Different compilers tolerate different (and incompatible) combinations of public and non-public decls and defns.  I think the patch as-is will compile with clang but not GCC, or maybe vice versa.

::: js/xpconnect/idl/xpcexception.idl
@@ +7,5 @@
>  #include "nsISupports.idl"
>  #include "nsIException.idl"
>  
>  %{ C++
> +#include "js/Value.h"

We can just forward declare JS::Value here.
Attachment #791893 - Flags: review?(n.nethercote)
Comment on attachment 792001 [details] [diff] [review]
eliminate jsapi.h from nsIScriptContext.h and nsIScriptRuntime.h

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

::: js/src/jspubtd.h
@@ +37,5 @@
>  struct Zone;
>  struct CompartmentOptions;
>  
>  class CallArgs;
> +class CompileOptions;

Again, this is JS_PUBLIC_API and so a non-public forward declaration causes problems with some compilers.
Attachment #792001 - Flags: review?(n.nethercote)
This is much the same as my earlier patch, except I haven't removed includes
from .cpp files.
Attachment #792016 - Flags: review?(wmccloskey)
I'm converting |jsval| to |JS::Value| in various places because |JS::Value| is
the preferred form, and I don't want to forward declare |jsval|.
Attachment #792018 - Flags: review?(wmccloskey)
This patch break some stuff related to structured cloning out of jsapi.h into
a new file js/public/StructuredClones.h.  This allows IPCMessageUtils.h to not
depend on jsapi.h.

I also moved the JSVAL_* constants into js/public/Value.h.  This avoided
another |#include "jsapi.h"| somewhere (I can't remember where now) and seems
sensible.

The bodies of functions like JS_ReadStructuredClone() are still in jsapi.cpp.
Perhaps they should be moved into a new file vm/StructuredClone.cpp?
Attachment #792021 - Flags: review?(wmccloskey)
This patch break some stuff related to the profiler out of jsapi.h into
a new file js/public/Profiler.h.  This allows PseudoStack.h to not
depend on jsapi.h.
Attachment #792024 - Flags: review?(wmccloskey)
Stats for parts 1--4:

                            rebuilt     includes
orig                        2640        94396
big-h                       2564        53500
big-idl                     2560        51273
j-StructuredClone           2539        49377
j-Profiler                  2319        48524

"rebuilt" is the number of files rebuilt after touching jsapi.h, which I measured during a REBUILD_CHECK=1 build.

"includes" is the number of times jsapi.h is included during a browser build, which I measure by adding a |#warning| statement to the top of jsapi.h and then counting how many times it gets printed.  This metric makes it clear how difficult this job is -- before my patches, the .cpp files that transitively #include jsapi.h do so on average 35.8(!) times each.  (Actually, the "rebuilt" includes not just .o builds but also some linking stuff, so the real number is actually slightly higher.)  With the four patches applied, that number is down to 20.9, and about 12% of them are actually down to zero, which is where they need to be for an effect to be noticed.

I will continue with this tomorrow.
Another thing:  jsfriendapi.h depends on jsclass.h, which depends on jsapi.h.  So, to decouple jsfriendapi.h from jsapi.h, we first have to decouple jsclass.h from jsapi.h.  I have a draft patch that successfully does that, which I'll clean up and upload tomorrow.
(In reply to Bill McCloskey (:billm) from comment #27)
>  1925  dom/base/nsJSEnvironment.h
>  1888  content/events/src/Touch.h
>  1834  widget/nsGUIEvent.h
>  1510  layout/base/nsIPresShell.h
>  1493  layout/base/nsPresContext.h

This chain should be fixable by moving the Touch constructors out of line. In fact, I thought I'd seen a patch for that... David, do you remember?

>  1788  objdir-ff-dbg/dom/bindings/DocumentBinding.h
>  1788  content/base/public/nsIDocument.h
>  1671  content/base/public/nsIContent.h
>  1440  content/base/src/nsNodeUtils.h

This one is more annoying, because nsIDocument.h needs an enum from DocumentBinding.h, and we're probably not going to get DocumentBinding.h off jsapi.h. Boris, do you have any ideas here?
> Boris, do you have any ideas here?

Well, forward-declaring the enum doesn't work so hot (see discussion in bug 886755).  

We could output separate headers for "types declared in this binding" (enums, etc) and "wrapping machinery for this binding" (which is what mostly needs jsapi).  It'd take some surgery on the codegen and still fail for various cases in which the "typed declared in this binding" file would need jsapi.h anyway.
(In reply to :Ms2ger from comment #38)
> (In reply to Bill McCloskey (:billm) from comment #27)
> >  1925  dom/base/nsJSEnvironment.h
> >  1888  content/events/src/Touch.h
> >  1834  widget/nsGUIEvent.h
> >  1510  layout/base/nsIPresShell.h
> >  1493  layout/base/nsPresContext.h
> 
> This chain should be fixable by moving the Touch constructors out of line.
> In fact, I thought I'd seen a patch for that... David, do you remember?
> 

I just landed a patch fixing this in bug 903283.
(In reply to Nicholas Nethercote [:njn] from comment #30)
> Comment on attachment 791893 [details] [diff] [review]
> fix a few .idl files and BindingDeclarations.h
> 
> Review of attachment 791893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jspubtd.h
> @@ +34,5 @@
> >  
> >  class JS_PUBLIC_API(AutoGCRooter);
> >  
> >  struct Zone;
> > +struct CompartmentOptions;
> 
> This is harder than it looks, because CompartmentOptions is JS_PUBLIC_API. 
> Different compilers tolerate different (and incompatible) combinations of
> public and non-public decls and defns.  I think the patch as-is will compile
> with clang but not GCC, or maybe vice versa.

Would it work to just wrap it in JS_PUBLIC_API? It looks like we do that directly above for AutoGCRooter.
Attachment #792016 - Flags: review?(wmccloskey) → review+
Comment on attachment 792018 [details] [diff] [review]
(part 2) - Minimize inclusions of JS engine headers in .idl files.

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

::: js/xpconnect/idl/nsIXPConnect.idl
@@ +18,5 @@
>  #include "nsIObjectOutputStream.idl"
>  #include "nsIObjectInputStream.idl"
>  
>  %{ C++
> +#include "jsapi.h"

What's this for? If we forward-declare a few things, we should be able to get away without this.
Attachment #792018 - Flags: review?(wmccloskey) → review+
Comment on attachment 792021 [details] [diff] [review]
(part 3) - Move structured clone stuff from jsapi.h to js/StructuredClone.h.

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

I find big comments more readable with the /* */ form, but I'm probably in the minority.
Attachment #792021 - Flags: review?(wmccloskey) → review+
Attachment #792024 - Flags: review?(wmccloskey) → review+
Attachment #791893 - Attachment is obsolete: true
Attachment #792001 - Attachment is obsolete: true
> ::: js/xpconnect/idl/nsIXPConnect.idl
> @@ +18,5 @@
> >  #include "nsIObjectOutputStream.idl"
> >  #include "nsIObjectInputStream.idl"
> >  
> >  %{ C++
> > +#include "jsapi.h"
> 
> What's this for? If we forward-declare a few things, we should be able to
> get away without this.

For CompartmentOptions.  I avoided removing it because of the JS_PUBLIC_API problems, but I'll dig a bit deeper and try to fix them.
Ok, I have it straight now.

(1) If clang sees a a non-PUBLIC forward declaration followed by a PUBLIC
    definition, it errors: "visibility does not match previous declaration".

(2) If GCC sees a PUBLIC definition followed by a PUBLIC forward declaration,
    it warns: "type attributes ignored after type is already defined".

When I tried to forward declare CompartmentOptions in nsIXPConnect.idl I always
got one of these, because some files include jsapi.h before nsIXPConnect.h, and
some do it in the other order.

But putting the forward declaration in jspubtd.h and making it PUBLIC ensures
that neither of the above cases happen -- (1) doesn't happen because the
forward declaration is public, and (2) doesn't happen because the forward
declaration is always seen before the definition, because jsapi.h includes 
jspubtd.h.  So I'll do that.
Same as before, except I moved the relevant function definitions from jsapi.cpp
to a new file vm/StructuredClone.cpp.
Attachment #792647 - Flags: review?(wmccloskey)
Attachment #792021 - Attachment is obsolete: true
Same as before, but I moved the relevant function definitions out of jsapi.h
into the new file js/Profiler.cpp.
Attachment #792649 - Flags: review?(wmccloskey)
Attachment #792024 - Attachment is obsolete: true
BTW, the patches previously called 3 and 4 are now called 2 and 3, because I've locally folded the patches that were previously called 1 and 2 into a single patch, for easier rebasing.

Updated numbers:

                            rebuilt     includes
orig                        2624        91457
big-h-idl                   2527        40826
j-StructuredClone           2505        39001
j-Profiler                  2228        38159

This is after removing the jsapi.h includes from nsIXPConnect.idl and nsIScriptContext.h.  So that's a 15% reduction in the number that matters.

I've been working through the compile errors on try server today, and am pretty close to everything building now.
Comment on attachment 792647 [details] [diff] [review]
(part 2) - Move structured clone stuff from jsapi.h to js/StructuredClone.h.

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

::: js/src/vm/StructuredClone.cpp
@@ +46,5 @@
> +    const JSStructuredCloneCallbacks *callbacks =
> +        optionalCallbacks ?
> +        optionalCallbacks :
> +        cx->runtime()->structuredCloneCallbacks;
> +    return WriteStructuredClone(cx, value, (uint64_t **) bufp, nbytesp,

While you're touching this code, bufp already is a uint64_t**
Here's the latest version of the .h+.idl files patch.  I'm getting a Windows
build error on try that I don't understand:

> DictionaryHelpers.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) bool __cdecl JS_GetPropertyById(struct JSContext *,class JSObject *,int,class JS::MutableHandle<struct JS::Value>)" (__imp_?JS_GetPropertyById@@YA_NPAUJSContext@@PAVJSObject@@HV?$MutableHandle@UValue@JS@@@JS@@@Z) referenced in function "enum tag_nsresult __cdecl DOMFileMetadataParameters_InitInternal(class mozilla::idl::DOMFileMetadataParameters &,struct JSContext *,class JS::Handle<class JSObject *>)" (?DOMFileMetadataParameters_InitInternal@@YA?AW4tag_nsresult@@AAVDOMFileMetadataParameters@idl@mozilla@@PAUJSContext@@V?$Handle@PAVJSObject@@@JS@@@Z)
> 
> DictionaryHelpers.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) bool __cdecl js::ToInt32Slow(struct JSContext *,class JS::Handle<struct JS::Value>,int *)" (__imp_?ToInt32Slow@js@@YA_NPAUJSContext@@V?$Handle@UValue@JS@@@JS@@PAH@Z) referenced in function "bool __cdecl JS::ToInt32(struct JSContext *,class JS::Handle<struct JS::Value>,int *)" (?ToInt32@JS@@YA_NPAUJSContext@@V?$Handle@UValue@JS@@@1@PAH@Z)
> 
> DictionaryHelpers.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) bool __cdecl js::ToInt64Slow(struct JSContext *,class JS::Handle<struct JS::Value>,__int64 *)" (__imp_?ToInt64Slow@js@@YA_NPAUJSContext@@V?$Handle@UValue@JS@@@JS@@PA_J@Z) referenced in function "bool __cdecl JS::ToInt64(struct JSContext *,class JS::Handle<struct JS::Value>,__int64 *)" (?ToInt64@JS@@YA_NPAUJSContext@@V?$Handle@UValue@JS@@@1@PA_J@Z)
> 
> DictionaryHelpers.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) bool __cdecl js::ToUint64Slow(struct JSContext *,class JS::Handle<struct JS::Value>,unsigned __int64 *)" (__imp_?ToUint64Slow@js@@YA_NPAUJSContext@@V?$Handle@UValue@JS@@@JS@@PA_K@Z) referenced in function "bool __cdecl JS::ToUint64(struct JSContext *,class JS::Handle<struct JS::Value>,unsigned __int64 *)" (?ToUint64@JS@@YA_NPAUJSContext@@V?$Handle@UValue@JS@@@1@PA_K@Z)

I'm getting these errors despite the fact that I modified
dictionary_helper_gen.conf so that jsapi.h is included in the list of files
included by DictionaryHelpers.cpp.  And I'm not getting errors for other
similar JS_PUBLIC_API functions such as JS_HasPropertyById() that are also used
in DictionaryHelpers.cpp.  Odd.
khuey offered to take a look at the Windows bustage.
Flags: needinfo?(khuey)
Attachment #792647 - Flags: review?(wmccloskey) → review+
Comment on attachment 792649 [details] [diff] [review]
(part 3) - Move profiler stuff from jsapi.h to js/Profiler.h.

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

::: js/ipc/JavaScriptShared.h
@@ +11,5 @@
>  #include "mozilla/dom/DOMTypes.h"
>  #include "mozilla/jsipc/PJavaScript.h"
>  #include "nsJSUtils.h"
>  #include "nsFrameMessageManager.h"
> +#include "jsclass.h"

What's this for?

::: js/src/vm/Profiler.cpp
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * vim: set ts=8 sts=4 et sw=4 tw=99:
> + * 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/. */

As we discussed, I think this should all go in one file.

@@ +12,5 @@
> +
> +using namespace js;
> +
> +JS_FRIEND_API(jsbytecode*)
> +ProfileEntry::pc() volatile {

The brace should be in a separate line. Can you fix this even if you don't move this function?

@@ +18,5 @@
> +    return idx == NullPCIndex ? NULL : script()->code + idx;
> +}
> +
> +JS_FRIEND_API(void)
> +ProfileEntry::setPC(jsbytecode *pc) volatile {

Same here.
Attachment #792649 - Flags: review?(wmccloskey) → review+
> ::: js/ipc/JavaScriptShared.h
> @@ +11,5 @@
> >  #include "mozilla/dom/DOMTypes.h"
> >  #include "mozilla/jsipc/PJavaScript.h"
> >  #include "nsJSUtils.h"
> >  #include "nsFrameMessageManager.h"
> > +#include "jsclass.h"
> 
> What's this for?

PseudoStack.h no longer includes jsfriendapi.h, which includes jsclass.h.  JavaScriptShared.h needs jsclass.h, and was previously somehow indirectly including it via PseudoStack.h.

The monster's claws are long and reach deeply, my child.

> ::: js/src/vm/Profiler.cpp
> 
> As we discussed, I think this should all go in one file.

Yep.  I'll rename js/Profiler.cpp as js/ProfilingStack.cpp, because that's more specifically what it's about.
There's a similar situation with patch 2, which leaves us with:
- jsclone.cpp
- jsclone.h
- js/StructuredClone.h
- vm/StructuredClone.cpp

Fortunately there's a neat solution.  I'll fold jsclone.cpp into vm/StructuredClone.cpp, and then jsclone.h is no longer needed.

billm, sound ok?
Flags: needinfo?(wmccloskey)
(In reply to Nicholas Nethercote [:njn] from comment #53)
> PseudoStack.h no longer includes jsfriendapi.h, which includes jsclass.h. 
> JavaScriptShared.h needs jsclass.h, and was previously somehow indirectly
> including it via PseudoStack.h.

But what code in JavaScriptShared.h requires jsclass.h?

> billm, sound ok?

Sure, that's fine.
Flags: needinfo?(wmccloskey)
> > DictionaryHelpers.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) bool __cdecl js::ToInt32Slow(...)

Where does the |__cdecl| come from?  That doesn't seem right.  My best guess is it's from one of these, from nscore.h:

#define EXPORT_XPCOM_API(type) NS_EXTERN_C NS_EXPORT type NS_FROZENCALL
#define IMPORT_XPCOM_API(type) NS_EXTERN_C NS_IMPORT type NS_FROZENCALL
#define GLUE_XPCOM_API(type) NS_EXTERN_C NS_HIDDEN_(type) NS_FROZENCALL

(NS_FROZENCALL expands to |__cdecl| on Windows.)
> Where does the |__cdecl| come from?

Or maybe that's auto-inserted by the compiler because these are C-ish functions (i.e. not class methods)?
> bool __cdecl JS_GetPropertyById(struct JSContext *,class JSObject *,int,class JS::MutableHandle<struct JS::Value>)"
> bool __cdecl js::ToInt32Slow(struct JSContext *,class JS::Handle<struct JS::Value>,int *)" 
> > js::ToInt64Slow(struct JSContext *,class JS::Handle<struct JS::Value>,__int64 *)
> bool __cdecl JS::ToUint64(struct JSContext *,class JS::Handle<struct JS::Value>,unsigned __int64 *)

One weird thing about the errors is that several other JSAPI functions used in
DictionaryHelpers.cpp aren't complained about:  JS_HasPropertyById(),
JS_InternString(), JS_ValueToBoolean().  The only difference of note between
those functions and the complained-about ones above is that the ones above each
have Handle<Value> or Mutable<Value> parameters.  I don't know why that would
make a difference, though.
Maybe it is no longer using C-like linkage when there's a templated argument?
> But what code in JavaScriptShared.h requires jsclass.h?

Nothing... but JavaScriptParent.h, which includes JavaScriptShared.h, uses ESClassValue, which is an enum and so can't be forward declared.

So I put the #include in the wrong file -- I'll move it.  Thanks!
Back to the Windows linking problem... I thought "what's changed about DictionaryHelpers.cpp?"  The only thing I can think of is that it's not including some headers that it used to.  It includes xpcprivate.h, and my patch removes inclusions of the following files from xpcpivate.h:  jsapi.h, jsdbgapi.h, jsfriendapi.h, jswrapper.h, js/HeapAPI.h.

So I tried adding them to DictionaryHelpers.cpp, by modifying dictionary_helper_gen.py.  But it didn't help.  Argh.
njn has a fix in hand that I verified locally.
Flags: needinfo?(khuey)
> njn has a fix in hand that I verified locally.

The fix was dumb and brutal:  I just reverted the changes to dictionary_helper_gen.{conf,py}.  DictionaryHelpers.h isn't included in many places so it doesn't matter much.  I still don't know exactly what the problem is;  something about which headers are included by DictionaryHelpers.{h,cpp} and their order.
Attachment #792016 - Attachment is obsolete: true
Attachment #792018 - Attachment is obsolete: true
Attachment #792798 - Flags: review+
Depends on: 908476
Depends on: 908489
> I'm afraid those broke at least one build:

I retriggered it this morning and it built fine.  (The try build I did just before landing was also fine.)  So presumably a clobber issue.  Sigh.
Depends on: 908537
https://hg.mozilla.org/mozilla-central/rev/781c291ec961
https://hg.mozilla.org/mozilla-central/rev/051e287b802f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.