550391 - Lots of MISMATCH_EXITS exits due to protohazardshape on voxel rendering demo

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

There are 93068 total trace exits in my run.  Of these:

76633 are MISMATCH_EXIT
15140 are LOOP_EXIT
  743 are NESTED_EXIT
  367 are OVERFLOW_EXIT
  143 are BRANCH_EXIT
   42 are TIMEOUT_EXIT

the mismatches (all the ones are checked) are due to protohazardshape guards in initprop.

So I looked into what might be bumping protohazardshape... and the answer seems to be "xpconnect".  Chrome, say.  In particular, getting an xpconnect method off its object makes the object a delegate (parents the method clone to the object), at least for non-helpered objects like most C++ xpcom stuff reflected into chrome JS.  Then getting a property adds the prop as shared (and often enough readonly) and we bump the protohazardshape.

So we're getting bumps from nsStandardURLs, nsWindowMediators, and the like.  nsPrefService.  nsJSID.  That sort of thing.

We need to either stop xpconnect doing this or stop mismatch_exiting on protoHazardShape bumps.  Or something.  This will hurt any script that interleaves with chrome code, as far as I can tell.

xref bug 503860, on brendan's request.

Comment 1

[Brendan Eich [:brendan]]

Couple of quick-fix thoughts: keep two protoHazardShapes (make it an array of length 2) indexed by obj->isSystem(). Second thought: try BRANCH_EXIT instead of MISMATCH_EXIT -- formally wrong since we do not expect predictable evolution of protoHazardShape, but worth a try? Might try both independently, just to get more data. bz, can do?

/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

So I tried doing the two protoHazardShapes.  Didn't help much, or at least not reliably, because DOM access to untrusted objects triggers the non-system shape bump (because the XPCNativeWrapper is not isSystem()).  Session store was poking the DOM in this case, for example.

Commenting the guard raises the fps from bouncing between 3 and 6 to reliably 6.

Making it a BRANCH_EXIT also gets me about 6fps; makes sense since I don't see much bumping of the protohazardshape during the testcase run.

I wish I understood why we have all the loop exits we do here...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Blake, are the new XRayWrappers isSystem()?  As in, should I try having two protoHazardShapes again?

Comment 4

[Brendan Eich [:brendan]]

The protoHazardShape runtime member gets regenerated when a non-writable data property or an accessor property with a setter is added to a delegate (prototype object, in practice -- scope objects don't generally get const or setter prosp).

Maybe we can fix this to happen less in XPConnect, using something like the fix for bug 610697. Boris, do you have some stacks handy showing regen of this shape?

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment 0 describes how it happens, no?  No handy stacks, but all the info is there.

Comment 6

[Andreas Gal :gal]

lets fix xpconnect to not require the parenting, except when its really needed (event handlers)

Comment 7

[Boris Zbarsky [:bzbarsky]]

So just parent to the global instead?  That's what we do for XBL methods.

That said, the event handler thing sounds fragile; basically if we trace some stuff and then someone sets up an event handler and then adds a prop, don't we lose?

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> That said, the event handler thing sounds fragile; basically if we trace some
> stuff and then someone sets up an event handler and then adds a prop, don't we
> lose?

We would if we mismatch-exit. Do we? Not saying that branch-exit is better at the limit, but it can be in practice.

We should try fixing things so the delegate bit is flipped only after the object has had functions defined on it, not when it is made the parent of those functions. IOW take setDelegate or its null-safe wrapper out of setParent. This requires some care to get right but it could be done.

/be

Comment 9

[Brendan Eich [:brendan]]

Sorry, read comment 0 finally -- ok, try branch exit as palliative, but let's do the real fix.

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

Doing both branch exit and better parenting of xpconnect methods sounds good to me.

Comment 11

[Brendan Eich [:brendan]]

Attachment: fix this boil on the engine's backside, for good I hope

Boris, could you please test this? Thanks,

/be

Comment 12

[Brendan Eich [:brendan]]

I think this bug should be wanted2.0+ or whatever it's called.

/be

Comment 13

[Brendan Eich [:brendan]]

I changed the last sentence of the big honking comment to this:

     * possibly a few old prehistoric scope objects (e.g. event targets).

/be

Comment 14

[Andreas Gal :gal]

I chatted with blake. We can easily fix this in xpc. We only use the parent because we don't have any other slot in which to store the object associated with the method. If we get another slot, we can parent to the global object and won't be setting the delegate bit any more.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

I worry that with this patch, we might end up creating the function's object with the wrong prototype (if cx->scopeChain()->getGlobal() doesn't match obj->getGlobal() -- we only assert that cx->scopeChain()->compartment() == obj->compartment(), which is weaker).

Comment 16

[Brendan Eich [:brendan]]

Andreas: this fix is for all embeddings and avoids bloating XPConnect with code to manage a different slot. It preserves API compatibility, except in keeping the delegate bit clear instead of set.

Blake: the patch here does not change anything from more than a day ago, except the lack of the delegate bit. I'm setting parent to obj, just as we have always (until yesterday on tm) done.

/be

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #16)
> Blake: the patch here does not change anything from more than a day ago, except
> the lack of the delegate bit. I'm setting parent to obj, just as we have always
> (until yesterday on tm) done.

Except that js_NewFunction is going to have to create its object, and pre-patch, you're passing in a non-null object (which is used by NewObject to find the prototype) but post-patch, you're passing in null.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> Except that js_NewFunction is going to have to create its object, and
> pre-patch, you're passing in a non-null object (which is used by NewObject to
> find the prototype) but post-patch, you're passing in null.

Argh, you are right. Fallback strategy: sample obj's delegate flag before the call to js_NewFunction, pass in obj, and then test whether delegate just got set, and clear it if so. This is gross.

/be

Comment 19

[Brendan Eich [:brendan]]

Attachment: lance the monkey-butt boil, but not my finger, v2

This requires taping the monkey to the table. Excuse the disgusting but apt metaphor.

/be

Comment 20

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/e0f43abe672f

/be

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Boris, could you please test this? Thanks,

On the original testcase in this bug, on trunk, we no longer hit the mismatch exits, even without these patches.  I have no idea why.

Comment 22

[Boris Zbarsky [:bzbarsky]]

On the testcase in bug 558469, without this patch about 20% of the side exits are MISMATCH exits.  With this patch there are no mismatch exits at all on that testcase.  So looks good!

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e0f43abe672f

Comment 24

[mdew]

Will this patch make it into trunk/4.0?

Comment 25

[Boris Zbarsky [:bzbarsky]]

That's what comment 23 was about, yes.  It's been on trunk for several days now.