Re: Stop #including jsapi.h everywhere!
I just was starting to look at BindingUtils.h as it is one of the most important hub headers that we have (see https://bugzilla.mozilla.org/show_bug.cgi?id=912735). But it seems that you guys are already well ahead into BindingUtils.h discussion. Is there a bug filed for it? Benoit 2013/8/21 Nicholas Nethercote n.netherc...@gmail.com On Wed, Aug 21, 2013 at 4:46 PM, Boris Zbarsky bzbar...@mit.edu wrote: On 8/21/13 2:23 AM, Nicholas Nethercote wrote: And jswrapper.h includes jsapi.h. I will try to remedy that... it looks doable. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
2013/9/7 Benoit Jacob jacob.benoi...@gmail.com I just was starting to look at BindingUtils.h as it is one of the most important hub headers that we have (see https://bugzilla.mozilla.org/show_bug.cgi?id=912735). But it seems that you guys are already well ahead into BindingUtils.h discussion. Is there a bug filed for it? Benoit Here are some patches towards making BindingUtils.h a cheaper header to include: https://bugzilla.mozilla.org/show_bug.cgi?id=913847 moves NS_IsMainThread to a new MainThreadUtils.h header that's cheaper to include, and in particular is all what BindingUtils.h needs (there was a helpful comment about that in BindingUtils.h). https://bugzilla.mozilla.org/show_bug.cgi?id=913852 makes BindingUtils.h not include algorithm just for one use of std::min. If there is a BindingUtils.h tracking bug, they could block it. Benoit ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On 9/7/13 12:56 PM, Benoit Jacob wrote: https://bugzilla.mozilla.org/show_bug.cgi?id=913847 moves NS_IsMainThread to a new MainThreadUtils.h header that's cheaper to include, and in particular is all what BindingUtils.h needs (there was a helpful comment about that in BindingUtils.h). Excellent. Note https://bugzilla.mozilla.org/show_bug.cgi?id=909971 also: we can stop including MainThreadUtils.h in this header too, I think. https://bugzilla.mozilla.org/show_bug.cgi?id=913852 makes BindingUtils.h not include algorithm just for one use of std::min. This is good, but unfortunately algorithm leaks in all over the place anyway in DOM code. The way it does that is that dom/Element.h has inline methods that need nsPresContext.h and Units.h. Either one will get you things like nsRect.h or nsCoord.h, both of which include algorithm. Oh, nsContentUtils.h includes Units.h too... We should strongly consider moving the Element methods that need those includes out of line, I think; not sure what we can do about nsContentUtils. If there is a BindingUtils.h tracking bug, they could block it. There isn't one yet. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
2013/9/7 Boris Zbarsky bzbar...@mit.edu On 9/7/13 12:56 PM, Benoit Jacob wrote: https://bugzilla.mozilla.org/**show_bug.cgi?id=913847https://bugzilla.mozilla.org/show_bug.cgi?id=913847 moves NS_IsMainThread to a new MainThreadUtils.h header that's cheaper to include, and in particular is all what BindingUtils.h needs (there was a helpful comment about that in BindingUtils.h). Excellent. Note https://bugzilla.mozilla.org/**show_bug.cgi?id=909971https://bugzilla.mozilla.org/show_bug.cgi?id=909971also: we can stop including MainThreadUtils.h in this header too, I think. Thanks for the link. MainThreadUtils.h is tiny, though, so this won't be a big deal anymore. https://bugzilla.mozilla.org/**show_bug.cgi?id=913852https://bugzilla.mozilla.org/show_bug.cgi?id=913852 makes BindingUtils.h not include algorithm just for one use of std::min. This is good, but unfortunately algorithm leaks in all over the place anyway in DOM code. The way it does that is that dom/Element.h has inline methods that need nsPresContext.h and Units.h. Either one will get you things like nsRect.h or nsCoord.h, both of which include algorithm. Oh, nsContentUtils.h includes Units.h too... Incidentally, nsRect.h just got fixed by https://bugzilla.mozilla.org/show_bug.cgi?id=913603 Thanks for pointing out nsCoord.h, let's fix it... (will file a bug and block the tracking bug 912735) Benoit We should strongly consider moving the Element methods that need those includes out of line, I think; not sure what we can do about nsContentUtils. If there is a BindingUtils.h tracking bug, they could block it. There isn't one yet. -Boris __**_ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/**listinfo/dev-platformhttps://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On Sun, Sep 8, 2013 at 2:38 AM, Benoit Jacob jacob.benoi...@gmail.com wrote: I just was starting to look at BindingUtils.h as it is one of the most important hub headers that we have (see https://bugzilla.mozilla.org/show_bug.cgi?id=912735). But it seems that you guys are already well ahead into BindingUtils.h discussion. Is there a bug filed for it? I've been focusing entirely on jsapi.h includes. We're down to ~1480 rebuilt files when it gets touched; we started at ~2600. It's getting hard to improve, because there's a tangled clump of widely-included files that include it still either directly or indirectly: BindingUtils.h, xpcpublic.h, nsCxPusher.h, DOMJSProxyHandler.h, Workers.h, probably a couple of others I've forgotten right now. For some of these it's not too hard to break the dependency, but AFAICT we need to break the dependency for most or all of them, which is harder. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On 9/7/13 8:49 PM, Nicholas Nethercote wrote: I've been focusing entirely on jsapi.h includes. We're down to ~1480 rebuilt files when it gets touched; we started at ~2600. It's getting hard to improve, because there's a tangled clump of widely-included files that include it still either directly or indirectly: BindingUtils.h, xpcpublic.h, nsCxPusher.h, DOMJSProxyHandler.h, Workers.h, probably a couple of others I've forgotten right now. For some of these it's not too hard to break the dependency, but AFAICT we need to break the dependency for most or all of them, which is harder. Of those 1480 files, 440 or so are generated binding .cpp files. You can't stop including jsapi.h in those, for obvious reasons. We should looks at non-binding files that include BindingUtils.h and figure out why: I suspect they mostly want UnwrapObject (which definitely needs jsapi-ish bits, but could have a non-inline version for those callers) or WrapObject (which I expect doesn't need JSAPI for its inline parts). DOMJSProxyHandler.h is included almost entirely in bindings code or via BindingUtils.h. I suspect xpcpublic.h is over-included. https://bugzilla.mozilla.org/show_bug.cgi?id=910937 kills off some of that, but I bet there's more we can get rid of. I believe nsCxPusher.h only needs jsapi.h because it needs to know sizeof(JSAutoRequest) and sizeof(JSAutoCompartment) for the members of AutoCxPusher... Not sure what we can do with that. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On Sun, Sep 8, 2013 at 11:02 AM, Boris Zbarsky bzbar...@mit.edu wrote: I believe nsCxPusher.h only needs jsapi.h because it needs to know sizeof(JSAutoRequest) and sizeof(JSAutoCompartment) for the members of AutoCxPusher... Not sure what we can do with that. One possibility is to move them into their own header(s) in js/public/. Another is to heap-allocate them, so we'd just need forward declarations in nsCxPusher.h. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On Tue, Aug 20, 2013 at 9:01 AM, Boris Zbarsky bzbar...@mit.edu wrote: BindingUtils.h we could try breaking up in various ways, but it should be very rare for _headers_ to include that file; for the most part such inclusions are a bug from my point of view. For non-headers that include it (e.g. binding implementation files), it might well be common to need jsapi.h. I have build-time instrumentation (using clang's -H option) that tells me which file is responsible for an include of another. (Because clang avoids reading a header more than once during the compilation of a single .cpp file, this only gives a partial picture, but it's still useful.) It tells me that BindingUtils.h is responsible for 824 inclusions of jswrapper.h during a build. And jswrapper.h includes jsapi.h. Here are the headers that include BindingUtils.h, and how often they are included during a build: dom/workers/FileReaderSync.h: 2 dom/workers/XMLHttpRequest.h: 6 dom/bindings/test/TestBindingHeader.h: 6 dom/bindings/PrimitiveConversions.h: 436 dom/indexedDB/IDBFactory.h: 12 dom/encoding/TextEncoderBase.h: 0 dom/encoding/TextDecoderBase.h: 0 dom/base/nsHistory.h: 3 js/xpconnect/src/XPCQuickStubs.h: 445 layout/style/nsICSSDeclaration.h: 470 content/media/webspeech/recognition/SpeechGrammarList.h: 7 content/canvas/src/WebGLContextUtils.h: 3 content/canvas/src/WebGLContext.h: 42 content/base/src/nsXMLHttpRequest.h: 14 content/base/src/WebSocket.h: 4 So: PrimitiveConversions.h, XPCQuickStubs.h, and nsICSSDeclaration.h look like the important ones. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On Wed, Aug 21, 2013 at 4:46 PM, Boris Zbarsky bzbar...@mit.edu wrote: On 8/21/13 2:23 AM, Nicholas Nethercote wrote: And jswrapper.h includes jsapi.h. I will try to remedy that... it looks doable. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On 2013-08-19 9:10 PM, Gregory Szorc wrote:
On 8/19/13 5:15 PM, Nicholas Nethercote wrote:
Hi,
Analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=901132 has
indicated that jsapi.h is probably responsible for more recompilation
than any other file in the Mozilla tree -- it gets included in *lots*
of files, and it gets modified very often, partly because it's so
large. (jsfriendapi.h is also bad in this respect.)
I'm trying to improve this situation in
https://bugzilla.mozilla.org/show_bug.cgi?id=905017. It turns out
that *many* |#include jsapi.h| statements are simply not needed.
They can be replaced by forward declarations of a handful of types,
e.g.:
struct jsid;
struct JSContext;
class JSObject;
namespace JS {
template typename T class Handle;
template typename T class MutableHandle;
class Value; // #include js/Value.h (not jsapi.h) if you need the
*definition*
}
Next time you're thinking of adding a |#include jsapi.h| statement,
please think about whether a forward declaration would suffice -- i.e.
if you are only using public JS types (i.e. not functions), and only
using them as pointers, references, or parameters in function
declarations. (Forward-declaring JS_PUBLIC_API types is harder; ask
me for help if you need to do that.)
[BTW, you might think these popular forward declarations should be in
a header of their own, to avoid some repetition. That's a reasonable
idea, and there is a file jspubtd.h that serves this purpose... sort
of. It contains numerous forward declarations (many more than is
needed in 98% of cases), but it also defines some types, so its
purpose is muddied. So I'm not doing that for now, though creating a
new file js/ForwardDecls.h (or somesuch) is a possibility in the
future.]
As well as using forward declarations where suitable, I've started
breaking off small sections of jsapi.h into separate headers in
js/public/. Unfortunately, this work will only go so far because
xpcpublic.h, BindingUtils.h, and DOMJSClass.h all (unavoidably)
include jsapi.h, and they are headers that are large and included in
lots of places. I'd love to hear suggestions as to how they could be
broken into smaller pieces and/or included in fewer places.
Issues like this could be identified and prevented in the future if we
had static analysis identifying badness. Is bug 887641 the only thing
blocking unhiding the static analysis builders on TBPL (887642)? Has
anyone filed bugs to get #include sanity checking added to our Clang
plugin?
In order to do that we would basically need to build a bug-free
include-what-you-use, and AFAIK nobody has signed up to do that work.
Also note that such a check will only be useful if we adhere to the
principle of using forward-decls where possible throughout the tree,
otherwise there will be thousands of known failures. We're very far
from that right now.
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On 8/19/13 8:15 PM, Nicholas Nethercote wrote: Unfortunately, this work will only go so far because xpcpublic.h, BindingUtils.h, and DOMJSClass.h all (unavoidably) include jsapi.h, and they are headers that are large and included in lots of places. I'd love to hear suggestions as to how they could be broken into smaller pieces and/or included in fewer places. DOMJSClass.h only needs various forward-declarations, mostly. The exceptions are: 1) It needs js::GetGlobalForObjectCrossCompartment (used in an inline method), but we could move that bit into some other header. The declaration of the inline method would need to stay here, but the definition could be somewhere else. 2) It needs the definition of JSClass, for a member of the DOMJSClass struct and the DOMIfaceAndProtoJSClass struct. Unfortunately, this is defined in jsapi.h and defining the DOMJSClass struct is the main point of this header file. 3) It needs js::GetReservedSlot for some inline functions, but we could put those in some other header. BindingUtils.h we could try breaking up in various ways, but it should be very rare for _headers_ to include that file; for the most part such inclusions are a bug from my point of view. For non-headers that include it (e.g. binding implementation files), it might well be common to need jsapi.h. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On 8/20/13 7:27 AM, Ehsan Akhgari wrote:
On 2013-08-19 9:10 PM, Gregory Szorc wrote:
On 8/19/13 5:15 PM, Nicholas Nethercote wrote:
Hi,
Analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=901132 has
indicated that jsapi.h is probably responsible for more recompilation
than any other file in the Mozilla tree -- it gets included in *lots*
of files, and it gets modified very often, partly because it's so
large. (jsfriendapi.h is also bad in this respect.)
I'm trying to improve this situation in
https://bugzilla.mozilla.org/show_bug.cgi?id=905017. It turns out
that *many* |#include jsapi.h| statements are simply not needed.
They can be replaced by forward declarations of a handful of types,
e.g.:
struct jsid;
struct JSContext;
class JSObject;
namespace JS {
template typename T class Handle;
template typename T class MutableHandle;
class Value; // #include js/Value.h (not jsapi.h) if you need the
*definition*
}
Next time you're thinking of adding a |#include jsapi.h| statement,
please think about whether a forward declaration would suffice -- i.e.
if you are only using public JS types (i.e. not functions), and only
using them as pointers, references, or parameters in function
declarations. (Forward-declaring JS_PUBLIC_API types is harder; ask
me for help if you need to do that.)
[BTW, you might think these popular forward declarations should be in
a header of their own, to avoid some repetition. That's a reasonable
idea, and there is a file jspubtd.h that serves this purpose... sort
of. It contains numerous forward declarations (many more than is
needed in 98% of cases), but it also defines some types, so its
purpose is muddied. So I'm not doing that for now, though creating a
new file js/ForwardDecls.h (or somesuch) is a possibility in the
future.]
As well as using forward declarations where suitable, I've started
breaking off small sections of jsapi.h into separate headers in
js/public/. Unfortunately, this work will only go so far because
xpcpublic.h, BindingUtils.h, and DOMJSClass.h all (unavoidably)
include jsapi.h, and they are headers that are large and included in
lots of places. I'd love to hear suggestions as to how they could be
broken into smaller pieces and/or included in fewer places.
Issues like this could be identified and prevented in the future if we
had static analysis identifying badness. Is bug 887641 the only thing
blocking unhiding the static analysis builders on TBPL (887642)? Has
anyone filed bugs to get #include sanity checking added to our Clang
plugin?
In order to do that we would basically need to build a bug-free
include-what-you-use, and AFAIK nobody has signed up to do that work.
Also note that such a check will only be useful if we adhere to the
principle of using forward-decls where possible throughout the tree,
otherwise there will be thousands of known failures. We're very far
from that right now.
I'm not talking about building a bug-free IWYU: I'm talking about making
the static analyzer be able to quickly identify suboptimal foo. e.g. we
would maintain a list of please forward declare symbols from jsapi.h.
At compile time if a file includes jsapi.h and only uses symbols from
the please forward declare list, then a warning is issued. We can put
the additional checks behind compiler flags/warnings and can ratchet up
the strictness over time. How will this not work?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On 08/20/2013 09:01 AM, Boris Zbarsky wrote: DOMJSClass.h only needs various forward-declarations, mostly. The exceptions are: 2) It needs the definition of JSClass, for a member of the DOMJSClass struct and the DOMIfaceAndProtoJSClass struct. Unfortunately, this is defined in jsapi.h and defining the DOMJSClass struct is the main point of this header file. We can/should add js/public/Class.h and move all the class gunk into that. I think the only dependencies of JSClass are various JSCLASS_* macros and the function typedefs, and those only depend on the rooting types and JSAPI structs that we can forward-declare. Jeff ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On 2013-08-20 12:52 PM, Gregory Szorc wrote: In order to do that we would basically need to build a bug-free include-what-you-use, and AFAIK nobody has signed up to do that work. Also note that such a check will only be useful if we adhere to the principle of using forward-decls where possible throughout the tree, otherwise there will be thousands of known failures. We're very far from that right now. I'm not talking about building a bug-free IWYU: I'm talking about making the static analyzer be able to quickly identify suboptimal foo. e.g. we would maintain a list of please forward declare symbols from jsapi.h. At compile time if a file includes jsapi.h and only uses symbols from the please forward declare list, then a warning is issued. We can put the additional checks behind compiler flags/warnings and can ratchet up the strictness over time. How will this not work? Yeah, we can probably make it work for a small subset of things, since we wouldn't need to address all imaginable cases for them. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On Tue, Aug 20, 2013 at 11:19 AM, Jeff Walden jwalden+...@mit.edu wrote: On 08/20/2013 09:01 AM, Boris Zbarsky wrote: DOMJSClass.h only needs various forward-declarations, mostly. The exceptions are: 2) It needs the definition of JSClass, for a member of the DOMJSClass struct and the DOMIfaceAndProtoJSClass struct. Unfortunately, this is defined in jsapi.h and defining the DOMJSClass struct is the main point of this header file. We can/should add js/public/Class.h and move all the class gunk into that. I think the only dependencies of JSClass are various JSCLASS_* macros and the function typedefs, and those only depend on the rooting types and JSAPI structs that we can forward-declare. There's a bunch of stuff in jsapi.h that will need to be moved into Class.h, but it's quite doable and on my short-term todo list. I might be able to remove jsfriendapi.h's dependency on jsapi.h as a result of this as well. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Stop #including jsapi.h everywhere!
Hi,
Analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=901132 has
indicated that jsapi.h is probably responsible for more recompilation
than any other file in the Mozilla tree -- it gets included in *lots*
of files, and it gets modified very often, partly because it's so
large. (jsfriendapi.h is also bad in this respect.)
I'm trying to improve this situation in
https://bugzilla.mozilla.org/show_bug.cgi?id=905017. It turns out
that *many* |#include jsapi.h| statements are simply not needed.
They can be replaced by forward declarations of a handful of types,
e.g.:
struct jsid;
struct JSContext;
class JSObject;
namespace JS {
template typename T class Handle;
template typename T class MutableHandle;
class Value; // #include js/Value.h (not jsapi.h) if you need the
*definition*
}
Next time you're thinking of adding a |#include jsapi.h| statement,
please think about whether a forward declaration would suffice -- i.e.
if you are only using public JS types (i.e. not functions), and only
using them as pointers, references, or parameters in function
declarations. (Forward-declaring JS_PUBLIC_API types is harder; ask
me for help if you need to do that.)
[BTW, you might think these popular forward declarations should be in
a header of their own, to avoid some repetition. That's a reasonable
idea, and there is a file jspubtd.h that serves this purpose... sort
of. It contains numerous forward declarations (many more than is
needed in 98% of cases), but it also defines some types, so its
purpose is muddied. So I'm not doing that for now, though creating a
new file js/ForwardDecls.h (or somesuch) is a possibility in the
future.]
As well as using forward declarations where suitable, I've started
breaking off small sections of jsapi.h into separate headers in
js/public/. Unfortunately, this work will only go so far because
xpcpublic.h, BindingUtils.h, and DOMJSClass.h all (unavoidably)
include jsapi.h, and they are headers that are large and included in
lots of places. I'd love to hear suggestions as to how they could be
broken into smaller pieces and/or included in fewer places.
Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
On Mon, Aug 19, 2013 at 05:15:13PM -0700, Nicholas Nethercote wrote:
Hi,
Analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=901132 has
indicated that jsapi.h is probably responsible for more recompilation
than any other file in the Mozilla tree -- it gets included in *lots*
of files, and it gets modified very often, partly because it's so
large. (jsfriendapi.h is also bad in this respect.)
I'm trying to improve this situation in
https://bugzilla.mozilla.org/show_bug.cgi?id=905017. It turns out
that *many* |#include jsapi.h| statements are simply not needed.
They can be replaced by forward declarations of a handful of types,
e.g.:
struct jsid;
struct JSContext;
class JSObject;
namespace JS {
template typename T class Handle;
template typename T class MutableHandle;
class Value; // #include js/Value.h (not jsapi.h) if you need the
*definition*
}
Next time you're thinking of adding a |#include jsapi.h| statement,
please think about whether a forward declaration would suffice -- i.e.
if you are only using public JS types (i.e. not functions), and only
using them as pointers, references, or parameters in function
declarations. (Forward-declaring JS_PUBLIC_API types is harder; ask
me for help if you need to do that.)
Note that here we need to decouple headers and source files. If foo.h
only needs forward declarations for itself, use forward declarations in
foo.h, and if foo.cpp does need more than forward declarations, include
jsapi.h in foo.cpp. Why? Because if you include jsapi.h in foo.h because
foo.cpp needs it, you're also including jsapi.h in anything that
includes foo.h, while they most likely don't need it.
(Note this applies to any types and headers, really, and we're doing a
lot of those #include bar.h in foo.h because Foo have a Bar* member)
Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Stop #including jsapi.h everywhere!
namespace JS {
class Value; // #include js/Value.h (not jsapi.h) if you need the
*definition*
}
Oh, and avoid |jsval| -- it's on the way out, and is just a typedef
for JS::Value, which should be used instead.
Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
