Closed
Bug 730961
Opened 12 years ago
Closed 12 years ago
GC: fix up indirect marking internals
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file)
12.13 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
Once all of the interfaces are fully indirect, we can carry the indirection all the way through to MarkInternal and avoid the extra stack store/load gymnastics we do currently.
Assignee | ||
Comment 1•12 years ago
|
||
For our verifier, it would be nice if the void** we get through our trace function were an exact match for the JSObject** stored in our remembered set. Currently, this is not possible because in MarkInternal we store the void* to the stack, then dereference it there. We need to do this because MarkInternal still takes a void*. We did this so that we could update for indirection one block of functions at a time, without having to touch other stuff. Now that this is done (except for 2 functions included in this patch), it is time to push the indirection all the way through to MarkInternal. Try run is at: https://tbpl.mozilla.org/?tree=Try&rev=a09d26b0ad1c
Attachment #606288 -
Flags: review?(wmccloskey)
Comment on attachment 606288 [details] [diff] [review] v0: don't pass objects through the stack Review of attachment 606288 [details] [diff] [review]: ----------------------------------------------------------------- Awesome. ::: js/src/jsgc.cpp @@ +2058,5 @@ > debugPrinter = elem->debugPrinter; > debugPrintArg = elem->debugPrintArg; > debugPrintIndex = elem->debugPrintIndex; > #endif > + MarkKind(this, &elem->thing, elem->kind); Please assert here that elem->thing doesn't change. Right now elem->thing is just a copy, so the original location won't be updated properly. @@ +2150,5 @@ > const char *name = entry.value.name ? entry.value.name : "root"; > + if (entry.value.type == JS_GC_ROOT_GCTHING_PTR) { > + void *tmp = *reinterpret_cast<void **>(entry.key); > + MarkGCThingRoot(trc, &tmp, name); > + JS_ASSERT(tmp == *reinterpret_cast<void **>(entry.key)); I think you should just be able to call: MarkGCThingRoot(trc, reinterpret_cast<void **>(entry.key), name); No need for the assert. @@ +2154,5 @@ > + JS_ASSERT(tmp == *reinterpret_cast<void **>(entry.key)); > + } else { > + Value tmp = *reinterpret_cast<Value *>(entry.key); > + MarkValueRoot(trc, &tmp, name); > + JS_ASSERT(tmp == *reinterpret_cast<Value *>(entry.key)); I don't think this code needed to change at all. ::: js/src/jsgcmark.cpp @@ +92,2 @@ > { > + JS_ASSERT(thingp); Can you copy *thingp to a local variable and use it throughout this function? @@ +257,4 @@ > { > JS_ROOT_MARKING_ASSERT(trc); > JS_SET_TRACING_NAME(trc, name); > + if (!thingp || !*thingp) I don't see any reason to allow thingp to be NULL. Let's just assert.
Attachment #606288 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 3•12 years ago
|
||
Previous try looks like it contained the MacOS build bustage from this morning. https://tbpl.mozilla.org/?tree=Try&rev=825630322f00
Assignee | ||
Comment 4•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/c3cf00d5514d
https://hg.mozilla.org/mozilla-central/rev/c3cf00d5514d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in
before you can comment on or make changes to this bug.
Description
•