887533 - Include even less stuff in dom/bindings

Status: RESOLVED FIXED

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

Description

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

Attachment: Patch

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

Comment 1

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

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm a little surprised that's enough.  Do we just not hit #2 and#3, or do they compile for some reason?

Comment 3

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

I think we just don't hit it.  The only thing with hasXPConnectImpls today is EventTarget.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, because we fixed Event, right.  And we have fewer and fewer external interfaces, I guess...

Alright, then!

Comment 5

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

Comment on attachment 768046 [details] [diff] [review]
Patch

I'm willing to just ignore the edge cases if you are ...

Comment 6

[Boris Zbarsky [: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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip, I think

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Actually compiling too

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Are there any more low-hanging #include fruit here?  ;)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: This should fix the test orange

Comment 11

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

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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Why do we need workerCallbacks here 

We don't.  Removed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/15380522db96

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15380522db96

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fwiw, this push reduced the memory needed to link libxul by about 120MB (!).

Comment 15

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

Not sure whether to be happy or befuddled.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Happy.  ;)

Other include-reductions are also producing drops in the libxul link memory, just smaller ones.

Comment 17

[(no longer active)]

(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!