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)

Other Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

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.
Blocks: LandIon
No longer blocks: IonMonkey
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)
Blocks: 748188
Blocks: 754491
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+
(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
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.