774757 - Paris Bindings cause static initializers

Status: RESOLVED FIXED

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

Description

[Matt Brubeck (:mbrubeck)]

This changeset from bug 749101 caused a regression of +1 in the "Number of Constructors" (static initializers) Talos measurement on CentOS x86 and x86_64:
https://hg.mozilla.org/mozilla-central/rev/dd08c10193c6

As I understand it, this does not cause a noticeable performance impact in the long run but it may block future startup optimizations, so we should track these regressions and understand how to fix them.  (I think filing a new bug for each regression will be more useful than just commenting in the original bug as I have in the past, but I'm open to feedback.)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I thought we already had a bug for the issue...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ms2ger, do you recall what has the initializer in this case?

Comment 3

[(no longer active)]

Hmm, isn't that number counting static objects with non-default constructors?  I don't see any such objects in the generated code here.

Also, that patch turned on the Paris bindings for three objects, I don't understand why it should only increment this counter by 1!

Comment 4

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

Isn't the static initializer for the cycle collector class?  Or did we get rid of those?

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> Isn't the static initializer for the cycle collector class?  Or did we get
> rid of those?

We did (bug 616262).

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Constants, I thought, but there are 4 of those...

Comment 7

[Matt Brubeck (:mbrubeck)]

Bug 753517 added 2 DOMInterfaces and 2 addExternalIface, and increased the constructor count by 3.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: patch

This patch introduces DOMProxyHandler::getInstance() for holding and returning the instance variable; it eliminates the associated static initializers.  I think this is more-or-less what we did for the CC static initializers, though I didn't follow that macro maze too closely.

CC'ing glandium to make sure I didn't screw up too badly, or if there's a better way to do this.

BTW, nice job on the bindings!  Very pleasant to add stuff.

Comment 9

[Nathan Froyd [:froydnj]]

Hm, that patch doesn't totally eliminate them; there's still MozXMLHttpRequestParameters::moz{Anon,System}_id, for instance.  I'll poke at that tomorrow.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Nathan Froyd (:froydnj) from comment #9)
> Hm, that patch doesn't totally eliminate them; there's still
> MozXMLHttpRequestParameters::moz{Anon,System}_id, for instance.  I'll poke
> at that tomorrow.

opt or debug builds? In opt builds, jsid is a numeric type, so should be fine, AIUI.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 656614 [details] [diff] [review]
patch

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

::: dom/bindings/Codegen.py
@@ +4883,5 @@
>  
> +class CGDOMJSProxyHandler_getInstance(ClassMethod):
> +    def __init__(self, descriptor):
> +        ClassMethod.__init__(self, "getInstance", "DOMProxyHandler*", [], static=True)
> +        self.descriptor = descriptor

You don't seem to use self.descriptor?

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to :Ms2ger from comment #11)
> > +class CGDOMJSProxyHandler_getInstance(ClassMethod):
> > +    def __init__(self, descriptor):
> > +        ClassMethod.__init__(self, "getInstance", "DOMProxyHandler*", [], static=True)
> > +        self.descriptor = descriptor
> 
> You don't seem to use self.descriptor?

Ah, yes, pasto.

(In reply to :Ms2ger from comment #10)
> (In reply to Nathan Froyd (:froydnj) from comment #9)
> > Hm, that patch doesn't totally eliminate them; there's still
> > MozXMLHttpRequestParameters::moz{Anon,System}_id, for instance.  I'll poke
> > at that tomorrow.
> 
> opt or debug builds? In opt builds, jsid is a numeric type, so should be
> fine, AIUI.

Ah, whoops!  This was a debug build.  Build what you want to measure and all that...

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #12)
> (In reply to :Ms2ger from comment #10)
> > (In reply to Nathan Froyd (:froydnj) from comment #9)
> > > Hm, that patch doesn't totally eliminate them; there's still
> > > MozXMLHttpRequestParameters::moz{Anon,System}_id, for instance.  I'll poke
> > > at that tomorrow.
> > 
> > opt or debug builds? In opt builds, jsid is a numeric type, so should be
> > fine, AIUI.
> 
> Ah, whoops!  This was a debug build.  Build what you want to measure and all
> that...

JFTR, the static initializers are still there...GCC's inability to optimize this shows up again...

Comment 14

[Nathan Froyd [:froydnj]]

Er.  For jsvals, not jsids.  But I suppose that's a known problem.

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: patch

Tidy up after Ms2ger's comments.

Comment 16

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

Comment on attachment 657269 [details] [diff] [review]
patch

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

I think bz or peterv should review this; I don't really know much about DOMProxyHandler (in particular whether there's one or multiple instances).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 657269 [details] [diff] [review]
patch

The only concern I have here is that IsProxy() can be pretty hot.  How expensive does getInstance() end up being with this change?  If it's too expensive, could we have a class-static pointer initially set to null that we compare against in IsProxy() and an ensureInstance() that we use for the NewProxyObject call that checks the class-static pointer and if not set sets it to point to a "stack" static?

r=me modulo that

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky (:bz) from comment #17)
> The only concern I have here is that IsProxy() can be pretty hot.  How
> expensive does getInstance() end up being with this change?  If it's too
> expensive, could we have a class-static pointer initially set to null that
> we compare against in IsProxy() and an ensureInstance() that we use for the
> NewProxyObject call that checks the class-static pointer and if not set sets
> it to point to a "stack" static?

Accesses to |instance| are guarded by a flag and associated locking to DTRT in the presence of multiple threads.  (At least on my Linux box; I assume other platforms do similar things).  The common (already-initialized) case should just be a couple of extra instructions (test flag and jump); the non-initialized case is somewhat more expensive (several dozen additional instructions in the presence of no thread contention), but I don't think there's a good way around that, and we'll only pay it once.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yeah, paying it once is fine.  If the cost is just a test and jump for the initialized read, that's ok.

Comment 20

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87196975aeea

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/87196975aeea