828753 - jsid rooting, mostly in jsinfer.*

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Somehow I got here from jsapi-tests crashes, but eneded up scanning through all of jsinfer.* and rooting everything not type-related. Mostly, it's jsid, but it covers some other stuff as well.

Comment 1

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

Attachment: jsid rooting, mostly in jsinfer.*

This is my first attempt. The major ugliness that you'll notice is a RawId version of getProperty().

Comment 2

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

Attachment: create HandleT from EncapsulatedT

For the next patch, I needed HandleId's of EncapsulatedId's. It was a bit more work than I expected.

Comment 3

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

Comment on attachment 700148 [details] [diff] [review]
jsid rooting, mostly in jsinfer.*

Ugh. Canceling review request for now. The getProperty stuff is totally unnecessary, since its id parameter doesn't need to be rooted. (getProperty() can GC, though.)

Comment 4

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

Comment on attachment 700149 [details] [diff] [review]
create HandleT from EncapsulatedT

This patch is no longer needed, though we may want it for other things.

Comment 5

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

Attachment: jsid rooting

Rewritten patch. Now standalone.

why has bzexport forsaken me??!

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 700589 [details] [diff] [review]
jsid rooting

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

Looks pretty reasonable now. Be sure and run this by Try before you push.

::: js/src/jsapi.h
@@ +2546,5 @@
>  
> +static jsid voidId = JSID_VOID;
> +static js::HandleId JSID_VOIDHANDLE = js::HandleId::fromMarkedLocation(&voidId);
> +static jsid emptyId = JSID_EMPTY;
> +static js::HandleId JSID_EMPTYHANDLE = js::HandleId::fromMarkedLocation(&emptyId);

I think these would read better with an underscore before HANDLE.

::: js/src/jsinfer.cpp
@@ +815,5 @@
>  {
>      AssertCanGC();
>      if (!obj->isNative())
>          return UnrootedShape(NULL);
> +    RootedId id(cx, DropUnrooted(idArg));

No need to DropUnrooted here.

@@ +3264,5 @@
>              /* Go through all shapes on the object to get integer-valued properties. */
>              UnrootedShape shape = singleton->lastProperty();
>              while (!shape->isEmptyShape()) {
> +                RawId propid = IdToTypeId(shape->propid());
> +                if (JSID_IS_VOID(propid))

It seems like it would be better to keep this on a single line.

@@ +3270,5 @@
>                  shape = shape->previous();
>              }
>          } else if (!JSID_IS_EMPTY(id) && singleton->isNative()) {
> +            RootedId rootedId(cx, id);
> +            UnrootedShape shape = singleton->nativeLookup(cx, rootedId);

Ditto.

::: js/src/jsinferinlines.h
@@ +290,5 @@
>       * and overflowing integers.
>       */
>      if (JSID_IS_STRING(id)) {
>          JSFlatString *str = JSID_TO_FLAT_STRING(id);
> +        const jschar *cp = str->charsZ();

Yup. Nice.

@@ +1553,5 @@
>          return NULL;
>      }
>  
>      if (!*pprop) {
> +        AssertCanGC();

We probably don't need to double up.

@@ +1559,2 @@
>          setBasePropertyCount(propertyCount);
> +        if (!addProperty(cx, DropUnrooted(id), pprop)) {

You also don't need to DropUnrooted here. I probably should have made the non Unrooted<T> variant unavailable in DEBUG.

::: js/src/methodjit/Compiler.cpp
@@ +5184,5 @@
>      if (types && !types->unknownObject() &&
>          types->getObjectCount() == 1 &&
>          types->getTypeObject(0) != NULL &&
>          !types->getTypeObject(0)->unknownProperties() &&
> +        id == types::IdToTypeId(id)) {

{ on new line.

Comment 7

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

Attachment: Misc rooting fixes exposed by the jsid patch

Followup fixes to jit-test breakage induced by the other patch.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 701309 [details] [diff] [review]
Misc rooting fixes exposed by the jsid patch

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

The IndexToId change isn't included in this patch?

Comment 9

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

Attachment: Misc rooting fixes exposed by the jsid patch

Sorry, the IndexToId stuff was some funky rebasing leftovers. (I had made that change in a separate patch that I tried to separate out, and somebody else made it upstream in the meantime.) So if you look at both patches, the first one does &id -> .address(), the second goes back the other way.

I've re-rebased to remove it from this patch. I'm still trying to get it through the try server; hitting warnings-as-errors stuff on Windows still. (And tbpl isn't talking to me atm, so I can't even fix the latest.)

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 701595 [details] [diff] [review]
Misc rooting fixes exposed by the jsid patch

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

::: js/src/ion/TypeOracle.cpp
@@ +301,5 @@
>      JSValueType idType = id->getKnownTypeTag();
>      if (idType != JSVAL_TYPE_INT32 && idType != JSVAL_TYPE_DOUBLE)
>          return false;
>  
> +    (void) DropUnrooted(script);

I'm not really liking the proliferation of DropUnrooted, particularly these bare ones that will make automatic removal of DropUnrooted hard. I think we should just make cases like this Raw, unless we expect the rooting analysis to have significant amounts of trouble detecting real failures.

Comment 11

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

Attachment: Misc rooting fixes exposed by the jsid patch

My apologies for this re-review request, but I ended up making a lot of changes to get this past the try server. Maybe Unrooted is more problematic than I thought. Also, this roots some stuff that may not need to be rooted if we disable GC during compilation entirely.

But it passes try, finally. (Well, with the next patch included, and I'll have to roll these up to commit them.)

Comment 12

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

Attachment: Misc rooting fixes exposed by the jsid patch

Whoops, forgot to remove some of the extra Unrooted/DropUnrooted pairs that you mentioned in an earlier review.

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 703379 [details] [diff] [review]
Misc rooting fixes exposed by the jsid patch

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

I'm getting a strong sense of deja-vu reading this.

Comment 14

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

Attachment: Make JSObject a class instead of a struct

Doh! Sorry, thought I posted this already. 3rd and final patch. This one is the most boring yet; I separated out the struct JSObject -> class JSObject patch.

All 3 patches together pass try, at least for the platforms that bothered to build: https://tbpl.mozilla.org/?tree=Try&rev=21eba4e2c8a7

Comment 15

[Terrence Cole [:terrence]]

Comment on attachment 703535 [details] [diff] [review]
Make JSObject a class instead of a struct

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

Exciting!

Comment 16

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

Combined patches pushed as:

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a5f5694ad2c0