Closed Bug 550391 Opened 10 years ago Closed 9 years ago

Lots of MISMATCH_EXITS exits due to protohazardshape on voxel rendering demo

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -
status2.0 --- wanted

People

(Reporter: bzbarsky, Assigned: brendan)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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.
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
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...
Blocks: 558469
Blake, are the new XRayWrappers isSystem()?  As in, should I try having two protoHazardShapes again?
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 0 describes how it happens, no?  No handy stacks, but all the info is there.
lets fix xpconnect to not require the parenting, except when its really needed (event handlers)
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?
(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
Sorry, read comment 0 finally -- ok, try branch exit as palliative, but let's do the real fix.

/be
Doing both branch exit and better parenting of xpconnect methods sounds good to me.
Boris, could you please test this? Thanks,

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #489549 - Flags: review?(mrbkap)
I think this bug should be wanted2.0+ or whatever it's called.

/be
blocking2.0: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
I changed the last sentence of the big honking comment to this:

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

/be
blocking2.0: ? → -
status2.0: --- → wanted
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.
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).
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
(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.
(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
This requires taping the monkey to the table. Excuse the disgusting but apt metaphor.

/be
Attachment #489549 - Attachment is obsolete: true
Attachment #489619 - Flags: review?(mrbkap)
Attachment #489549 - Flags: review?(mrbkap)
Attachment #489619 - Flags: review?(mrbkap) → review+
http://hg.mozilla.org/tracemonkey/rev/e0f43abe672f

/be
Whiteboard: fixed-in-tracemonkey
> 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.
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!
http://hg.mozilla.org/mozilla-central/rev/e0f43abe672f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Will this patch make it into trunk/4.0?
That's what comment 23 was about, yes.  It's been on trunk for several days now.
You need to log in before you can comment on or make changes to this bug.