139243 - xpconnect should not do JS rooting for wrapped natives

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[John Bandhauer]

bug 112152 talks about improving threadsafety for wrapped JS objects and getting
rid of JS rooting there. That is kind of tough. But, we can pretty easily kill
the need to do refcount based calls to add and remove JS roots for wrapped
*natives*.

Instead of rooting we can do timely marking. This is better because it avoids
the lock overhead inherent in the JS rooting call. And, it only happens at GC
time rather than each time the refcount on the wrapped natives makes a 1->2 or
2->1 transition.

We might see some measureable improvement in DOM manipulation benchmarks.

I'll attach a patch. I was working on dealing with bug 112152 too. So, this
patch has some associated code that will be needed for that fix, but which is
not *strictly* needed yet for this smaller fix. We should test this more and see
if anything unexpectedly expodes :)

Comment 1

[John Bandhauer]

Attachment: proposed fix

Comment 2

[Johnny Stenback (:jst)]

Attachment: Perf testcase

This testcase accesses document.nodeType in a tight loop and measures the time
it takes. Accessing document ends up wrapping the document object and also
QI'ing the wrapper in the security manager, so the code that's modified with
this patch is hit quite a lot.

Initial tests show a 9% performance imporvment with this testcase when applying
this patch. Nice :-)

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 80412 [details] [diff] [review]
proposed fix

sr=jst, makes sense to me.

Comment 4

[Brendan Eich [:brendan]]

Do we want this for 1.0RC2?  Propose it to drivers for pre-approval and linking
to bug 138000 if so.

/be

Comment 5

[David Bradley]

Comment on attachment 80412 [details] [diff] [review]
proposed fix

r=dbradley

Is accessing the reference count in HasExternalReferences portable, that is (is
the atomic guarentee for every type of access, or just increment/decrement).

The added assertion in GetNewOrUsed will be a good diagnostic

Comment 6

[John Bandhauer]

AFAIK reading a 32bit member is portable in the sense that it always gives a
coherent result. The reason we can avoid locking here is that the only
transition of the value we would not want to race with is the transition from 1
to 2. But, these are wrapped natives and that transition could only happen in
the GetNewOrUsed code that happens within a JS request - i.e. this would be
serialized with the GC.

Comment 7

[John Bandhauer]

I checked this into the trunk. If no problems come up soon I'll push for branch
approval.

Comment 8

[John Bandhauer]

I see that the trunk tinderbox speed tests had a small but measurable
improvement (about 1%) across the board with this checkin. No leak changes (and
I saw no new leaks in my local tests either).

Comment 9

[Johnny Stenback (:jst)]

Sweet!

Comment 10

[John Bandhauer]

I'm reassigning to jst. The change has been on the trunk for quite a while now.
It works well with no problems at all. It is a small change really. This is
primarily a DOM performance fix - though it seems to be an overall speedup.
AFAIK, no one has specifically tested the improvement of startup and new window
creation. They might be significant. jst can push for it getting into the branch
at some point if he wants.

Comment 11

[Peter Van der Beken [:peterv]]

Should just mark this fixed, it was landed ages ago.