Closed
Bug 748986
Opened 12 years ago
Closed 12 years ago
IonMonkey: Marking phase should mark gcThings given as arguments of VMFunction.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(1 file)
27.25 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Currently callVM is creating HandleObject by taking reference to JSObject* stored on the stack. These HandleObject have not been created by a Root and thus JSObject* need to be marked. One possible approach is to add a mark function to VMFunction/VMwrapper which should be called in MarkIonJSFrame to mark the arguments of the wrapper.
Good catch - yeah, let's do that.
Assignee | ||
Comment 2•12 years ago
|
||
Clean-up the exit frame footer and how it is manipulated. Add an extra field for the VMFunction pointer. This pointer is used to mark the VM wrapper arguments while marking the exit frame.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Attachment #623206 -
Flags: review?(dvander)
Comment on attachment 623206 [details] [diff] [review] Mark VM wrapper arguments. Review of attachment 623206 [details] [diff] [review]: ----------------------------------------------------------------- Nice work - as mentioned in IRC you could simplify things by reducing the number of mark types, but it's not strictly necessary. Could we get a follow-up bug for marking the vp on native calls? ::: js/src/ion/IonFrames.cpp @@ +516,5 @@ > + argBase += sizeof(void *); > + break; > + case VMFunction::DoubleByValue: > + case VMFunction::DoubleByRef: > + argBase += 2 * sizeof(void *); += sizeof(double) ? ::: js/src/ion/arm/IonFrames-arm.h @@ +181,2 @@ > } > + inline uint8 *argBase() { Here and on x86 - I think we should get a short comment explaining exactly what argsBase points to, and why it's safe to use it for marking (I think the reason is that Handles are byref, so argsBase points to the actual pushes in CodeGen).
Attachment #623206 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to David Anderson [:dvander] from comment #3) > Nice work - as mentioned in IRC you could simplify things by reducing the > number of mark types, but it's not strictly necessary. 01:53:12 <pierron> terrence: MarkGCThingRoot is safe for marking Value, JSObject* or JSString* ? 01:53:37 <terrence> pierron: no/what to you mean? 01:54:25 <terrence> pierron: you certainly cannot mark Value through that interface... it knows nothing about FatValue packing 01:54:30 <pierron> terrence: is there a generic function for marking, and is it better to store the type information to call the right marking function or to let the GC guess the type of it ? 01:54:37 <terrence> pierron: you can mark any Cell* through there 02:03:23 <pierron> billm: What is best, using MarkGCThingRoot or any specialized version of it ? 02:04:11 <billm> pierron: it's better to use MarkObjectRoot instead of MarkGCThingRoot, for example 02:04:28 <billm> pierron: faster and more readable 02:05:28 <pierron> dvander`home: ^ ? 02:05:54 <billm> pierron: sometimes you don't know the thing kind, and then you should use MarkGCThingRoot 02:05:55 <pierron> billm: I guess the same hold for MarkGCThingOrValueRoot ? 02:06:23 <billm> pierron: yeah, that's a hack for ionmonkey. please don't use it unless absolutely necessary. So, I will keep the current version for now. > Could we get a > follow-up bug for marking the vp on native calls? I opened Bug 754491 for it. > ::: js/src/ion/IonFrames.cpp > @@ +516,5 @@ > > + argBase += sizeof(void *); > > + break; > > + case VMFunction::DoubleByValue: > > + case VMFunction::DoubleByRef: > > + argBase += 2 * sizeof(void *); > > += sizeof(double) ? Apparently we will have to rename it, but this is not a “double” but a Double sized word.
Hrm, I don't understand why MarkGCThingRoot is more readable than breaking down all of these cases. IonMonkey relies heavily on the GC's ability to grab the type of a cell and this seems like an important feature of the GC. This is also not a hot path in the slightest. Anyway, the patch is fine as-is, so I don't have any issue breaking down the types - I just think it would be simpler not to do so (as the rest of IonMonkey doesn't).
Err, that should read: "isn't more readable", or something
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/53a84ecbb030
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•