1112934 - Make MGetDOMMember able to do typed slot loads

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Similar to what we do with MLoadFixedSlot.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Tell MGetDOMMember what its result type is so it can do a typed slot load

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Jan de Mooij [:jandem]]

Don't forget to push this after bug 1112563 part 2 lands..

Comment 4

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf65998682c

Comment 5

[Phil Ringnalda (:philor)]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Jan de Mooij [:jandem]]

(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.

Comment 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[Terrence Cole [:terrence]]

Attachment: autoleavezeal-v0.diff

(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.

Comment 10

[Phil Ringnalda (:philor)]

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

Comment 11

[Phil Ringnalda (:philor)]

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

Comment 12

[Phil Ringnalda (:philor)]

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/.

Comment 13

[Jan de Mooij [:jandem]]

(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

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c614bf536b5f
https://hg.mozilla.org/mozilla-central/rev/70d349ef50b7
https://hg.mozilla.org/mozilla-central/rev/3db31fa7d623
https://hg.mozilla.org/mozilla-central/rev/1a5309ae3125

Comment 15

[Steve Fink [:sfink] [:s:]]

(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.)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Phil, do you recall why you added leave-open here?  Afaict this is fixed, right?

Comment 17

[Phil Ringnalda (:philor)]

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.