860572 - Add specializations of Handle and MutableHandle for JSPropertyDescriptor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The idea is that:

1) Callees should be able to tell when their desc is rooted.
2) They should be able to easily get a MutableHandle<Value> for the .value of a
   MutableHandle<JSPropertyDescriptor>.  And perhaps similar for the .obj.  What
   to do with the getter and setter.... <sigh>.  I have no idea for those.

Comment 1

[Terrence Cole [:terrence]]

Attachment: v0

This should be all that's needed.

https://tbpl.mozilla.org/?tree=Try&rev=b42ebc59fbcc

Comment 2

[Boris Zbarsky [:bzbarsky]]

I guess we'll need a followup to actually use this, right?

Comment 3

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> I guess we'll need a followup to actually use this, right?

Yes, and some fixes to this patch as well. It seems there is a good reason I don't normally code when tired.

Comment 4

[Terrence Cole [:terrence]]

Attachment: v1: Add helper methods and use it a bit.

Build succeeded locally, so off to try again:
https://tbpl.mozilla.org/?tree=Try&rev=83688476493f

Comment 5

[Terrence Cole [:terrence]]

Fixed clang errors:
https://tbpl.mozilla.org/?tree=Try&rev=3a4dc299d3f1

Comment 6

[Terrence Cole [:terrence]]

Attachment: v2

And fix a namespace error. I hope this is the last try run we'll need:
https://tbpl.mozilla.org/?tree=Try&rev=63905e8538cb

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 737032 [details] [diff] [review]
v2

Actually, lets add the helper methods to Handle and MutableHandle too.

Comment 8

[Terrence Cole [:terrence]]

Attachment: 1 of 2: v3

Implements HandleBase and MutableHandleBase as well.

Comment 9

[Terrence Cole [:terrence]]

Attachment: 2 of 2: v0 - Use Rooted<JSPropertyDescriptor> in XPCComponents.

Rooted adds helper methods to JSPropertyDescriptor to make using it less error-prone. This is an example of such usage.

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 737058 [details] [diff] [review]
1 of 2: v3

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

::: js/src/gc/RootMarking.cpp
@@ +67,5 @@
>        case THING_ROOT_VALUE:       MarkValueRoot(trc, (Value *)addr, "exact-value"); break;
>        case THING_ROOT_ID:          MarkIdRoot(trc, (jsid *)addr, "exact-id"); break;
>        case THING_ROOT_PROPERTY_ID: MarkIdRoot(trc, &((js::PropertyId *)addr)->asId(), "exact-propertyid"); break;
>        case THING_ROOT_BINDINGS:    ((Bindings *)addr)->trace(trc); break;
> +      case THING_ROOT_BINDINGS:    ((JSPropertyDescriptor *)addr)->trace(trc); break;

I think you mean THING_ROOT_PROPERTY_DESCRIPTOR here.

::: js/src/gc/Verifier.cpp
@@ +37,3 @@
>          return address == static_cast<void*>(w);
>  
>      Bindings *bp = static_cast<Bindings*>(address);

This assumes address is always a Binding which is now not the case any more.

::: js/src/jsapi.h
@@ +3328,5 @@
> +    JS::MutableHandleObject obj() { return JS::MutableHandleObject::fromMarkedLocation(&desc()->obj); }
> +    JS::MutableHandleValue value() { return JS::MutableHandleValue::fromMarkedLocation(&desc()->value); }
> +    bool hasGetterObject() const { return desc()->attrs & JSPROP_GETTER; }
> +    bool hasSetterObject() const { return desc()->attrs & JSPROP_SETTER; }
> +    JSPropertyOp getter() const { MOZ_ASSERT(!hasGetterObject()); return desc()->getter; }

The assertion condition looks like it's the wrong way round to me (same on next line).

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #10)

> > +    JSPropertyOp getter() const { MOZ_ASSERT(!hasGetterObject()); return desc()->getter; }
> 
> The assertion condition looks like it's the wrong way round to me (same on
> next line).

Disregard this one, I realised that getter/setter are used for different types depending on the flags in attrs.

One more thing though, in the XrayWrapper code it would be useful to have a way to get attrs, as well as to set getter and setter.

Comment 12

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #10)
> ::: js/src/gc/RootMarking.cpp
> @@ +67,5 @@
> >        case THING_ROOT_VALUE:       MarkValueRoot(trc, (Value *)addr, "exact-value"); break;
> >        case THING_ROOT_ID:          MarkIdRoot(trc, (jsid *)addr, "exact-id"); break;
> >        case THING_ROOT_PROPERTY_ID: MarkIdRoot(trc, &((js::PropertyId *)addr)->asId(), "exact-propertyid"); break;
> >        case THING_ROOT_BINDINGS:    ((Bindings *)addr)->trace(trc); break;
> > +      case THING_ROOT_BINDINGS:    ((JSPropertyDescriptor *)addr)->trace(trc); break;
> 
> I think you mean THING_ROOT_PROPERTY_DESCRIPTOR here.

D'oh!
 
(In reply to Jon Coppeard (:jonco) from comment #11)
> (In reply to Jon Coppeard (:jonco) from comment #10)
> Disregard this one, I realised that getter/setter are used for different
> types depending on the flags in attrs.

Yeah, this is, in my opinion, the worst GC issue in SpiderMonkey: for example, for Shapes we also have to special-case the post barrier because of this.

A quick survey of PropertyDescriptor seems to show that all of the SpiderMonkey uses are on the stack as well. I'll file a new bug to remove the AutoRooter.

> One more thing though, in the XrayWrapper code it would be useful to have a
> way to get attrs, as well as to set getter and setter.

Good point! In the long run we'd like JSPropertyDescriptor to match the spec's concept of a PropertyDescriptor. However, after talking this over with Waldo, we concluded that this is long enough off that a suite of dumb wrappers of the existing functionality would be beneficial.

Comment 13

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c849d14e7f59
https://hg.mozilla.org/integration/mozilla-inbound/rev/d02e437f5b48

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=63905e8538cb

Comment 14

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/732e0bc6aff2 - backed out 2 patches for OSX build breakage

Comment 15

[Terrence Cole [:terrence]]

https://tbpl.mozilla.org/?tree=Try&rev=d3572d84f825

Comment 16

[Terrence Cole [:terrence]]

Okay, not really sure why the compiler only reported one of the two name-masking errors here. Lets spin the warning-as-errors wheel again:
https://tbpl.mozilla.org/?tree=Try&rev=de71aa1c35c9

Comment 17

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/49dbf1510125
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e1939f325abb

The full Try run appears to be green:
https://tbpl.mozilla.org/?tree=Try&rev=de71aa1c35c9

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49dbf1510125
https://hg.mozilla.org/mozilla-central/rev/e1939f325abb