Closed Bug 887533 Opened 7 years ago Closed 7 years ago

Include even less stuff in dom/bindings

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: khuey, Assigned: khuey)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Attached patch PatchSplinter Review
This only implements #1 from https://bugzilla.mozilla.org/show_bug.cgi?id=876555#c8 but it appears to be enough.
Before

Kyle Huey@KYLEHUEY-PC obj-i686-pc-mingw32/dom/bindings/.deps
$ cat *.obj.pp | wc -l
469262

After

Kyle Huey@KYLEHUEY-PC obj-i686-pc-mingw32/dom/bindings/.deps
$ cat *.obj.pp | wc -l
327406
I'm a little surprised that's enough.  Do we just not hit #2 and#3, or do they compile for some reason?
I think we just don't hit it.  The only thing with hasXPConnectImpls today is EventTarget.
Ah, because we fixed Event, right.  And we have fewer and fewer external interfaces, I guess...

Alright, then!
Comment on attachment 768046 [details] [diff] [review]
Patch

I'm willing to just ignore the edge cases if you are ...
Attachment #768046 - Flags: review?(bzbarsky)
Comment on attachment 768046 [details] [diff] [review]
Patch

Hmm.  So those #includes are needed because those types are the return values of GetParentObject?

I guess I don't have a better proposal for those, short of reducing the number of types we ever use for parent objects (basically to nsINode and nsPIDOMWindow or something) and including them in all all the binding cpp files.

But I guess that for something like nsDOMAttributeMap even if we wanted to return nsINode* we'd need to include Element.h so that we know Element inherits from nsINode.  :(

Another option is to out-of-line GetParentObject methods as needed, make them return ParentObject, and then the implementation is the only thing that needs to know about the actual types involved.  This does mean an extra out-of-line call when wrapping, though.

r=me either way.
Attachment #768046 - Flags: review?(bzbarsky) → review+
Attached patch Merged to tip, I think (obsolete) — Splinter Review
Without this patch, I get build times like so for dom/bindings:

1117.313u 100.303s 5:08.81 394.2%       0+0k 115+8662io 0pf+0w
1121.414u 100.267s 5:09.96 394.1%       0+0k 352+7266io 0pf+0w

With the patch I get build times like so:

661.799u 63.282s 3:05.31 391.2% 0+0k 1241+5981io 390pf+0w
644.380u 63.480s 3:00.86 391.3% 0+0k 442+6365io 0pf+0w

I filed bug 901299 on maybe fixing up the GetParentObject pain points.
Attachment #785473 - Attachment is obsolete: true
Are there any more low-hanging #include fruit here?  ;)
Blocks: includehell
Comment on attachment 794708 [details] [diff] [review]
This should fix the test orange

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

::: dom/bindings/Codegen.py
@@ +8639,5 @@
> +                return
> +            needsDOMQS["value"] = typeDesc.hasXPConnectImpls
> +
> +        callForEachType(descriptors + callbackDescriptors, dictionaries,
> +                        workerCallbacks + mainCallbacks,

Why do we need workerCallbacks here if only mainthread things can have hasXPConnectImpls?
Attachment #794708 - Flags: review?(khuey) → review+
> Why do we need workerCallbacks here 

We don't.  Removed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/15380522db96
https://hg.mozilla.org/mozilla-central/rev/15380522db96
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Fwiw, this push reduced the memory needed to link libxul by about 120MB (!).
Not sure whether to be happy or befuddled.
Happy.  ;)

Other include-reductions are also producing drops in the libxul link memory, just smaller ones.
(In reply to comment #14)
> Fwiw, this push reduced the memory needed to link libxul by about 120MB (!).

Yeah, it looks like the PGO compiler in the first phase just dumps out the entire AST into the object files, or something crazy like that!
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.