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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

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?
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.
Attachment #794504 - Flags: review?(bugs) → review+
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 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+
Attachment #794505 - Flags: review?(bugs) → review+
> 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)
Attachment #794510 - Flags: review?(bugs) → review+
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+
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.  ;)
Attachment #794783 - Flags: review?(bugs)
> And I think dict ctors could be out-of-line.

OK, done.
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+
> Yeah, we should move the nice fast hashtables to mfbt... ;)

mfbt/Vector.h!
> > Yeah, we should move the nice fast hashtables to mfbt... ;)
> 
> mfbt/Vector.h!

Oh, you said hashtables.  Sorry :/
The patches certainly don't make this testcase any worse...
Depends on: 909328
Good news!  Part 7 reduced the number of files that get rebuilt on my machine when jsapi.h is touched from 2223 to 1558.
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.
> 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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.