Closed Bug 1112934 Opened 5 years ago Closed 5 years ago

Make MGetDOMMember able to do typed slot loads

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

Similar to what we do with MLoadFixedSlot.
Comment on attachment 8538614 [details] [diff] [review]
Tell MGetDOMMember what its result type is so it can do a typed slot load

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

Looks good.
Attachment #8538614 - Flags: review?(jdemooij) → review+
Don't forget to push this after bug 1112563 part 2 lands..
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6c4b30ffe4d4 for orange in SM(r) - "try: -b do -p linux64 -u none -t none" and then kill the browser builds is the magic phrase to get it to run on try. Well, probably just -b d, but since I finally succeeded in getting it to run on my fourth set of backout tries, I didn't want to do any more experimenting.
Terrence, do you have any idea how this jit change could possibly affect testGCHeapPostBarriers and in particular make the return value of NurseryObject() test false for IsInsideNursery?  Or better yet, how I can run this test locally (on Mac?) in a way that matches whatever SM(r) is doing?
Flags: needinfo?(terrence)
(In reply to Boris Zbarsky [:bz] from comment #6)
> Or better yet, how I can
> run this test locally (on Mac?) in a way that matches whatever SM(r) is
> doing?

Build a debug + opt shell and run the test with JS_GC_ZEAL=7.
Oh, this is a jsapi-test, seems pretty unlikely this patch could trigger that. The SM(r) builds can be brittle, we've had similar issues before. Maybe do some Try pushes later and see if the failure is still there? Or ask sfink for help reproducing this somewhere.
(In reply to Boris Zbarsky [:bz] from comment #6)
> Terrence, do you have any idea how this jit change could possibly affect
> testGCHeapPostBarriers and in particular make the return value of
> NurseryObject() test false for IsInsideNursery?  Or better yet, how I can
> run this test locally (on Mac?) in a way that matches whatever SM(r) is
> doing?

There are two possibilities. Either, JS_NewObject(cx, 0, 0, 0) is no longer allocating in the nursery in the first place, or JS_DefineProperty(cx, obj, "x", 42, 0) is somehow causing a minor GC and tenuring obj before we run the test. When I built this test, my thinking was that shapes are not in the nursery so this couldn't trigger a minor gc, however, zealous gc seems to make that untrue. I'd guess that some unlucky change to allocation patterns is at fault here.

I think the right solution is for that particular test (and any other that depend on gc behavior) to temporarily disable the zeal GCs. This patch should do the trick.
Flags: needinfo?(terrence)
Attachment #8543524 - Flags: review?(sphink)
Blocks: 1114064
Comment on attachment 8543524 [details] [diff] [review]
autoleavezeal-v0.diff

Review is nice, but so is not having to back out every single js/src/ patch. Once I finally backed out bz's other patch because without this one the fragile ggc started failing, the very next push which landed caused testGCHeapPostBarriers to fail again.

Landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/c614bf536b5f
Attachment #8543524 - Flags: checkin+
Keywords: leave-open
Comment on attachment 8538614 [details] [diff] [review]
Tell MGetDOMMember what its result type is so it can do a typed slot load

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/70d349ef50b7
Attachment #8538614 - Flags: checkin+
Comment on attachment 8543524 [details] [diff] [review]
autoleavezeal-v0.diff

bz's followup to this in https://hg.mozilla.org/integration/mozilla-inbound/rev/3db31fa7d623 made it actually compile when JS_GC_ZEAL isn't defined, which is nice, but now there's the static analysis bustage of https://treeherder.mozilla.org/logviewer.html#?job_id=5057337&repo=mozilla-inbound so I chose the alternative of just leaving mozilla-inbound closed until someone fixes that, rather than backing out five pushes and then continuing to back out every patch that touches js/src/.
(In reply to Phil Ringnalda (:philor) from comment #12)
> so I chose the alternative of just leaving mozilla-inbound closed
> until someone fixes that, rather than backing out five pushes and then
> continuing to back out every patch that touches js/src/.

This might help (marking AutoLeaveZeal's constructor explicit).

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a5309ae3125
Attachment #8543524 - Flags: review?(sphink) → review+
(In reply to Jan de Mooij [:jandem] from comment #8)
> Oh, this is a jsapi-test, seems pretty unlikely this patch could trigger
> that. The SM(r) builds can be brittle, we've had similar issues before.
> Maybe do some Try pushes later and see if the failure is still there? Or ask
> sfink for help reproducing this somewhere.

For the record, the best way to reproduce this failure locally, and indeed any SM(...) failure, is to run js/src/devtools/automation/autospider.sh <variant>. For SM(r), that means

  js/src/devtools/automation/autospider.sh rootanalysis

That will do a build and run tests with the appropriate settings. In this case, the main thing is to run with JS_GC_ZEAL=7. And for this particular problem, where exact patterns of other stuff trigger failures, it can be handy to do eg JS_GC_ZEAL=7,N and vary N (the number after the comma means to do the zeal testing every N allocations, defaulting to 100.)
Phil, do you recall why you added leave-open here?  Afaict this is fixed, right?
Flags: needinfo?(philringnalda)
My best guess would be "I'm landing this without review, so it shouldn't be marked as fixed until after it actually gets review, at which point I'll depend on either the reviewer or the assignee resolving it." Probably.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(philringnalda)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.