Closed
Bug 852802
Opened 11 years ago
Closed 11 years ago
Add incremental needsBarrier to the runtime and and check it first
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file)
16.99 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
This lets us squelch pre-barriers before they access the zone pointer. This fixes two hard problems: (1) With generational GC, objects will get their Zone* by looking at Shape's arena header. During GC this shape may already have been finalized, so making this access is bad. (2) When doing a minor collection, marking a key in a hash table may find a moved object. The ::remove function of all of our hash tables correctly calls the pre-barrier. Since the pre-barrier does not know about relocations, it will look through a broken Shape* for the arena header. Currently we hack around these behaviors manually in all the places that happen to hit these cases by either doing a reinterpret_cast to change to an unbarriered type for the relevant accesses or calling a special method on the hash table. My measurements show no change in performance from this patch.
Attachment #727002 -
Flags: review?(wmccloskey)
Assignee | ||
Updated•11 years ago
|
Assignee: general → terrence
Comment on attachment 727002 [details] [diff] [review] v0 Review of attachment 727002 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/HeapAPI.h @@ +101,5 @@ > +GetGCThingRuntime(const void *thing) > +{ > + uintptr_t addr = uintptr_t(thing); > + addr &= ~js::gc::ChunkMask; > + addr += js::gc::ChunkSize - sizeof(JS::shadow::Runtime *); Could you instead create a ChunkRuntimeOffset (similar to ChunkMarkBitmapOffset) and use that here? Also, you should use |= instead of += in case the compiler fails to optimize it. Finally, please assert in gc/Heap.h that the runtime actually is at ChunkRuntimeOffset (similar to what we do for ChunkMarkBitmapOffset). ::: js/src/jsgc.cpp @@ +3239,5 @@ > #endif > } > > static void > +AssertNeedsBarriersAreConsistent(JSRuntime *rt) How about AssertNeedsBarrierFlagsConsistent? ::: js/src/jspubtd.h @@ +312,2 @@ > { > + bool needsBarrier_; This isn't good. I think you should be able to move JS::shadow::Runtime to this file and include jspubtd.h from gc/HeapAPI.h. Then you can make RuntimeDummy inherit from JS::shadow::Runtime.
Attachment #727002 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 2•11 years ago
|
||
Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=a189abf38b60 Pushed at: https://hg.mozilla.org/integration/mozilla-inbound/rev/3af927a8260c
Assignee | ||
Comment 3•11 years ago
|
||
Backed out in: https://hg.mozilla.org/integration/mozilla-inbound/rev/b839ca9cf702
Assignee | ||
Comment 4•11 years ago
|
||
Bill spotted the bug. Relanded in: https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6a4d28e510 (Full) green try run at: https://tbpl.mozilla.org/?tree=Try&rev=b44adec02570
Comment 5•11 years ago
|
||
Backed out for build bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/46b8016553fa https://tbpl.mozilla.org/php/getParsedLog.php?id=21313754&tree=Mozilla-Inbound ../../../js/src/jsgc.cpp:3202:52: error: 'void js::AssertNeedsBarrierFlagsConsistent(JSRuntime*)' should have been declared inside 'js'
Assignee | ||
Comment 6•11 years ago
|
||
Bustage is from removal of Try assertion. Trivial fix. Re-re-pushed at: https://hg.mozilla.org/integration/mozilla-inbound/rev/a95fe57e30de
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a95fe57e30de
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•