Closed
Bug 908576
Opened 11 years ago
Closed 11 years ago
Stop including DOMJSClass.h and DOMJSProxyHandler.h in binding headers
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(8 files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details |
The main goal here is actually to stop binding headers from including PrototypeList.h and jsapi.h, not least because nsIDocument has to include a binding header. Right now any time we add a binding everything that includes nsIDocument rebuilds!
The only parts of this that worry me are:
1) Making GetProtoObject/GetConstructorObject non-inline. I think this is actually OK.
2) Some possible extra relocations for the NativePropertyHooks bits.
3) Bindings with dictionaries will still need jsapi.h in the header. See the XXX comment... Should we fix that by making the dictionary initializer ctor out of line?
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #794504 -
Flags: review?(bugs)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #794505 -
Flags: review?(bugs)
Assignee | ||
Comment 3•11 years ago
|
||
Oh, and:
4) Now PrototypeList.h forward-declares a ton of stuff, which gives us some not-fully-qualified namespace collisions that the patches have to deal with.
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #794506 -
Flags: review?(bugs)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #794507 -
Flags: review?(bugs)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #794510 -
Flags: review?(bugs)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #794528 -
Flags: review?(bugs)
Updated•11 years ago
|
Attachment #794504 -
Flags: review?(bugs) → review+
Comment 8•11 years ago
|
||
Comment on attachment 794507 [details] [diff] [review]
part 4. Make GetProtoObject and GetConstructorObject not inline so they don't force us to include PrototypeList.h in all binding headers.
Do we have any way to test whether this affects to perf significantly.
Attachment #794507 -
Flags: review?(bugs) → review+
Comment 9•11 years ago
|
||
Comment on attachment 794528 [details] [diff] [review]
part 6. Move the various DOMProxyHandler classes into the binding implementation files.
It is super odd that nsNthIndexCache needs any JS.
(looks like the HashMap usage was added in Bug 598832.)
Attachment #794528 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Attachment #794505 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 10•11 years ago
|
||
> Do we have any way to test whether this affects to perf significantly.
Maybe. The main thing it would affect is creation of new wrappers, so we could try timing that... I'll give it a shot.
> It is super odd that nsNthIndexCache needs any JS.
Yeah, we should move the nice fast hashtables to mfbt... ;)
So I've run into a weird problem. This doesn't build on Windows, with this build error:
c:\program files (x86)\windows kits\8.0\include\um\msxml.h(10085) : error C2371:
'XMLDocument' : redefinition; different basic types
presumably due to the forward-declarations we're adding... but those are namespaced, no? So what gives? See try push at https://tbpl.mozilla.org/?tree=Try&rev=9a5c9244abea
Flags: needinfo?(khuey)
Updated•11 years ago
|
Attachment #794510 -
Flags: review?(bugs) → review+
Comment 11•11 years ago
|
||
Comment on attachment 794506 [details] [diff] [review]
part 3. Move all the PrototypeTraits and PrototypeIDMap bits into PrototypeList.h so they don't force us to include that header in all binding headers.
This one is mostly rs+.
And I think dict ctors could be out-of-line.
Attachment #794506 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 12•11 years ago
|
||
So I thought about it and instead of forward-declaring the world I'm just going to nuke the PrototypeIDMap structs and switch their consumers to a different setup. I've talked both bent and khuey into it, so it must be good. Or at least not too bad. ;)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #794783 -
Flags: review?(bugs)
Assignee | ||
Comment 14•11 years ago
|
||
> And I think dict ctors could be out-of-line.
OK, done.
Comment 15•11 years ago
|
||
Comment on attachment 794783 [details] [diff] [review]
Get rid of PrototypeIDMap.
I think I managed to understand the change :)
(Had to look at some generated files)
Attachment #794783 -
Flags: review?(bugs) → review+
Comment 16•11 years ago
|
||
> Yeah, we should move the nice fast hashtables to mfbt... ;)
mfbt/Vector.h!
Comment 17•11 years ago
|
||
> > Yeah, we should move the nice fast hashtables to mfbt... ;)
>
> mfbt/Vector.h!
Oh, you said hashtables. Sorry :/
Assignee | ||
Comment 18•11 years ago
|
||
The patches certainly don't make this testcase any worse...
Assignee | ||
Comment 19•11 years ago
|
||
Assignee | ||
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/215ef4dcf5ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/63fcffd683c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/382ba444845a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9842da20e726
https://hg.mozilla.org/integration/mozilla-inbound/rev/676307efdd97
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd3c54248f94
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e3075e7320
Flags: needinfo?(khuey) → in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla26
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/215ef4dcf5ba
https://hg.mozilla.org/mozilla-central/rev/63fcffd683c8
https://hg.mozilla.org/mozilla-central/rev/382ba444845a
https://hg.mozilla.org/mozilla-central/rev/9842da20e726
https://hg.mozilla.org/mozilla-central/rev/676307efdd97
https://hg.mozilla.org/mozilla-central/rev/fd3c54248f94
https://hg.mozilla.org/mozilla-central/rev/08e3075e7320
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 22•11 years ago
|
||
Good news! Part 7 reduced the number of files that get rebuilt on my machine when jsapi.h is touched from 2223 to 1558.
Blocks: minimize-jsapi
Assignee | ||
Comment 23•11 years ago
|
||
That was the idea, yes. ;)
Note that we're still including RootingAPI.h in all the binding headers, whereas I suspect we mostly need forward-declarations there.
Comment 24•11 years ago
|
||
> That was the idea, yes. ;)
It's very easy to reduce the number of transitive includes of jsapi.h without reducing the number of files that depend on it. E.g. if file X.h used to transitively include jsapi.h 20 times, and we reduce it to 10, that doesn't help incremental builds because there's still a dependency. So it's worth understanding and highlighting the times when you can get that number down to 0 for a significant file.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•