Last Comment Bug 748986 - IonMonkey: Marking phase should mark gcThings given as arguments of VMFunction.
: IonMonkey: Marking phase should mark gcThings given as arguments of VMFunction.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: LandIon 748188 754491
  Show dependency treegraph
 
Reported: 2012-04-25 14:49 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-05-11 22:39 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Mark VM wrapper arguments. (27.25 KB, patch)
2012-05-11 10:36 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-04-25 14:49:40 PDT
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.
Comment 1 David Anderson [:dvander] 2012-04-26 08:38:55 PDT
Good catch - yeah, let's do that.
Comment 2 Nicolas B. Pierron [:nbp] 2012-05-11 10:36:08 PDT
Created attachment 623206 [details] [diff] [review]
Mark VM wrapper arguments.

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.
Comment 3 David Anderson [:dvander] 2012-05-11 16:41:03 PDT
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).
Comment 4 Nicolas B. Pierron [:nbp] 2012-05-11 17:13:26 PDT
(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.
Comment 5 David Anderson [:dvander] 2012-05-11 17:21:48 PDT
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).
Comment 6 David Anderson [:dvander] 2012-05-11 17:22:57 PDT
Err, that should read: "isn't more readable", or something
Comment 7 Nicolas B. Pierron [:nbp] 2012-05-11 22:39:46 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/53a84ecbb030

Note You need to log in before you can comment on or make changes to this bug.