Thread more handles through jsarray

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 644519 [details] [diff] [review]
v0

Steve, do these changes make sense, or am I being too aggressive?
Attachment #644519 - Flags: review?(sphink)
Comment on attachment 644519 [details] [diff] [review]
v0

Review of attachment 644519 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsarray.cpp
@@ +3742,5 @@
>          }
>      }
>  
>      Rooted<GlobalObject*> parent(cx, parent_);
> +    RootedObject proto(cx, protoArg);

Ooh, that's a good example of a tricky case. You're double-rooting proto, but I think it's probably best this way. The inner proto that you pass to FindProto definitely has to be rooted, but the parameter doesn't need to be -- looking at the callers, you're now explicitly rooting and so you could avoid one root by passing in a bare JSObject* and rooting it at the top. But I like it the way you did it better, just for the simplicity; you don't have to think it through this way.

It's a good example for a rooting document.
Attachment #644519 - Flags: review?(sphink) → review+
Comment on attachment 644519 [details] [diff] [review]
v0

Review of attachment 644519 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsarray.cpp
@@ +3795,5 @@
>  #ifdef JS_METHODJIT
>  JSObject * JS_FASTCALL
>  mjit::stubs::NewDenseUnallocatedArray(VMFrame &f, uint32_t length)
>  {
> +    JSObject *obj = NewArray<false>(f.cx, length, HandleObject::fromMarkedLocation((JSObject **)&f.scratch));

Is this safe? f.scratch is not a marked location, as far as I can tell.
(Assignee)

Comment 3

5 years ago
Created attachment 645010 [details] [diff] [review]
v1

Steve, I've updated this to use RawObject, and I think it's better overall now. I do want a second opinion, however, so I'm re-requesting.

(In reply to Bill McCloskey (:billm) from comment #2)
> HandleObject::fromMarkedLocation((JSObject **)&f.scratch));
> 
> Is this safe? f.scratch is not a marked location, as far as I can tell.

I assumed wrong.  Do we need these to be marked through for moving GC?
Assignee: general → terrence
Attachment #644519 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #645010 - Flags: review?(sphink)
Whiteboard: [js:t]
Comment on attachment 645010 [details] [diff] [review]
v1

Review of attachment 645010 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about missing the f.scratch thing earlier; I made the wrong assumption too. I see now that it is a void* slot. Unfortunately, it seems to be used to store things like argc at times, so we can't just change it to a gc::Cell* and trace it.

::: js/src/jsarray.cpp
@@ +3683,5 @@
>      }
>  
>      Rooted<GlobalObject*> parent(cx, parent_);
> +    RootedObject proto(cx, protoArg);
> +    PoisonPtr(reinterpret_cast<uintptr_t *>(protoArg));

Ooh, we should fix that. The reinterpret_cast kind of defeats the purpose, since this is mainly for documentation. Would it work to just declare PoisonPtr as taking a void*, or do we need to templafuscate it?
Attachment #645010 - Flags: review?(sphink) → review+
(Assignee)

Comment 5

5 years ago
Making PoisonPtr take a void* worked like a charm:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6291bb72294

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/b6291bb72294
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.