Closed
Bug 952818
Opened 10 years ago
Closed 10 years ago
GenerationalGC: Assertion failure: isTenured(), at gc/Heap.h
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: gkw, Assigned: terrence)
References
Details
(Keywords: assertion, regression, testcase)
Attachments
(4 files, 2 obsolete files)
5.17 KB,
text/plain
|
Details | |
57.14 KB,
patch
|
Details | Diff | Splinter Review | |
54.25 KB,
patch
|
Details | Diff | Splinter Review | |
643 bytes,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(function() { eval("\ (function() {\ var f = function(){\ f\ }\ })()\ ") })() asserts js debug shell on m-c changeset 599100c4ebfe with --ion-parallel-compile=off --ion-eager --ion-check-thread-safety at Assertion failure: isTenured(), at gc/Heap.h My configure flags are: AR=ar sh ./configure --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-exact-rooting --enable-elf-hack --enable-stdcxx-compat --enable-warnings-as-errors --enable-signmar --disable-elf-hack --enable-js-diagnostics --with-intl-api=build --enable-ctypes --disable-shared-js --enable-jemalloc --enable-gcgenerational --with-ccache --enable-threadsafe <other NSPR options> autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/06f844b81f3d user: Brian Hackett date: Wed Nov 27 16:28:57 2013 -0700 summary: Bug 938124 - Add classes for use in IonBuilder thread safety analysis, r=jandem. Bug 938124 was probably the changeset that added --ion-check-thread-safety, so I'm not sure if it's the regressor.
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)
Comment 1•10 years ago
|
||
This reproduces in non-optimised debug builds also. IonBuilder::hasStaticScopeObject() encounters a call object in the nursery when traversing the current script's function's scope chain. I don't think this should be touching any nursery pointers here. The CallObject::create() implementations tenure the object if treatAsRunOnce() is true for the script, and this is checked in hasStaticScopeObject(). However this is the outer script only - from what I can see the inner script can still have a nursery call object. Brian, do you know how this is supposed to work?
Assignee: nobody → jcoppeard
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)
Flags: needinfo?(bhackett1024)
Comment 2•10 years ago
|
||
hasStaticScopeObject doesn't work with generational GC, as there is no guarantee the scope objects being traversed are not in the nursery. The attached patch restructures the addendums attached to type objects so that the type object for a run once function can hold pointers to the single call object for it. This also uses the addendum for a type object's interpreted function, which allows cutting sizeof(TypeObject) by two words on 32 bit platforms.
Assignee: jcoppeard → bhackett1024
Attachment #8355549 -
Flags: review?(jdemooij)
Flags: needinfo?(bhackett1024)
Assignee | ||
Updated•10 years ago
|
Assignee: bhackett1024 → terrence
Comment 3•10 years ago
|
||
Comment on attachment 8355549 [details] [diff] [review] patch Review of attachment 8355549 [details] [diff] [review]: ----------------------------------------------------------------- Terrence, can you land this patch before it bitrots (more)? It would be good to check Octane just to be sure. ::: js/src/jit/Ion.cpp @@ +1611,4 @@ > { > JS_ASSERT(obj->hasSingletonType()); > > + // Keep track of This should probably be: "Keep track of run-once call objects." or something. @@ +1614,5 @@ > + // Keep track of > + JS_ASSERT(obj->callee().hasSingletonType()); > + types::TypeObject *type = obj->callee().getType(cx); > + if (type && !type->hasRunOnceCallObject()) > + type->initAddendum(types::TypeObject::RunOnceCallObject, obj); It would be nice to assert either here or in initAddendum that obj is not allocated in the nursery. ::: js/src/jsinfer.cpp @@ +3011,5 @@ > + JS_ASSERT(!(flags() & OBJECT_FLAG_ADDENDUM_MASK)); > + JS_ASSERT(addendum); > + > + JS_ASSERT(kind <= OBJECT_FLAG_ADDENDUM_MASK >> OBJECT_FLAG_ADDENDUM_SHIFT); > + this->flags_ |= (kind << OBJECT_FLAG_ADDENDUM_SHIFT); Nit: add a JS_ASSERT(addendumKind() == kind); after this line -- it's too easy to break this when moving flags around etc.
Attachment #8355549 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8355549 [details] [diff] [review] patch Review of attachment 8355549 [details] [diff] [review]: ----------------------------------------------------------------- We need this patch to happen and Brian is out until February, so I am going to drive it forward. I've read it over carefully now and I think it's a great cleanup that we should get in asap. This probably still deserves a look from Jan, so I'll upload a rebased version in a bit with, the test, and my own review commentary included. ::: js/src/jit/Ion.cpp @@ +1611,4 @@ > { > JS_ASSERT(obj->hasSingletonType()); > > + // Keep track of Looks like this comment got chopped off. It's pretty clear what's happening here, so I don't think a comment is really even needed. ::: js/src/jit/IonBuilder.cpp @@ +9398,5 @@ > // so there will be only one call object and the aliased var access can be > + // compiled in the same manner as a global access. Look for the call > + // object, which was hopefully installed by TrackCallObjectProperties. > + > + *pcall = funType->runOnceCallObject(); \o/ Nice! ::: js/src/jsinfer.cpp @@ +3012,5 @@ > + JS_ASSERT(addendum); > + > + JS_ASSERT(kind <= OBJECT_FLAG_ADDENDUM_MASK >> OBJECT_FLAG_ADDENDUM_SHIFT); > + this->flags_ |= (kind << OBJECT_FLAG_ADDENDUM_SHIFT); > + this->addendum = addendum; This needs a post-barrier barrier or an assertion that addendum is not in the nursery. @@ +3028,5 @@ > } > > /* > + * It is possible for the object to not have a new script addendum yet, but > + * but to have one added in the future. When analyzing properties of new Remove the second "but".
Attachment #8355549 -
Flags: review?(jdemooij)
Attachment #8355549 -
Flags: review+
Assignee | ||
Comment 5•10 years ago
|
||
Having mid-aired with Jan in reviewing this, I'm a bit terrified at how identical our reviews are.
Attachment #8355549 -
Attachment is obsolete: true
Attachment #8355549 -
Flags: review?(jdemooij)
Attachment #8356740 -
Flags: review+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/12a28278bb20
Comment 7•10 years ago
|
||
I think awfy doesn't really like this commit. I see a 27% decrease on octane-typearray...
Assignee | ||
Comment 8•10 years ago
|
||
And backed out for adding 3 new hazards. https://hg.mozilla.org/integration/mozilla-inbound/rev/23e682c34d75 (In reply to Hannes Verschore [:h4writer], pto 13th - 17th of January from comment #7) > I think awfy doesn't really like this commit. I see a 27% decrease on > octane-typearray... Ouch! I'll take a look at that too.
(In reply to Terrence Cole [:terrence] from comment #8) > And backed out for adding 3 new hazards. > https://hg.mozilla.org/integration/mozilla-inbound/rev/23e682c34d75 > > (In reply to Hannes Verschore [:h4writer], pto 13th - 17th of January from > comment #7) > > I think awfy doesn't really like this commit. I see a 27% decrease on > > octane-typearray... > > Ouch! I'll take a look at that too. Looks like it also broke m4: https://tbpl.mozilla.org/php/getParsedLog.php?id=32657616&tree=Mozilla-Inbound And m-oth: https://tbpl.mozilla.org/php/getParsedLog.php?id=32658511&tree=Mozilla-Inbound
Assignee | ||
Comment 10•10 years ago
|
||
The perf problem is that we are not performing the new analysis during non-compilation analysis or before inlining. Fixing those two oversights results in a consistent 1% speed boost; I have no idea why this is significantly faster as it's just a refactoring. Still, I'll take it.
Assignee | ||
Comment 11•10 years ago
|
||
This is still "crashing" with signal 11 in the middle of browser tests. I've reprod, but I can't figure out what's going on: the code continues fine and seems to pass the test if I just ignore the signal by continuing. I managed to get TBPL to crash in a shell test as well, but I haven't been able to repro that locally yet. https://tbpl.mozilla.org/?tree=Try&rev=24b7a152cb7e
Assignee | ||
Comment 12•10 years ago
|
||
Since Try ate my patch, let's re-upload it to the bug.
Attachment #8356740 -
Attachment is obsolete: true
Attachment #8370199 -
Flags: feedback?(bhackett1024)
Comment 13•10 years ago
|
||
The reason that mochitest 4 and other were crashing is that TypeObject::sizeOfExcludingThis() was passing non-malloc-allocated addendum pointers to the mallocSizeOf function. I changed this to return zero when then addendum is a GC thing, as in that case the memory will be accounted for elsewhere. I put back in most of the commented-out optimisations, but two of them cause a problem. Calling TrackPropertiesForSingletonScopes() can acquire the compilation lock, so calling this from places in IonBuilder where the lock is already taken is a problem. This happens in hasStaticScopeObject() and canInlineTarget(). The comments say the lock is not reentrant. I don't understand the locking scheme enough to know how to proceed here. Without these two optimisations however the mochitests are green.
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8370199 [details] [diff] [review] do_runonce_computation_on_main_thread-v2.diff This doesn't reproduce without off-main-thread ionbuilder.
Attachment #8370199 -
Flags: feedback?(bhackett1024)
Assignee | ||
Comment 15•10 years ago
|
||
Let's add the regression test from this bug so that this doesn't get dropped when we re-enable threading in IonBuilder.
Attachment #8379150 -
Flags: review?(sphink)
Updated•10 years ago
|
Attachment #8379150 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a444cf2a9b09
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a444cf2a9b09
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•10 years ago
|
QA Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•