Closed Bug 828753 Opened 7 years ago Closed 7 years ago

jsid rooting, mostly in jsinfer.*

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(3 files, 5 obsolete files)

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.
This is my first attempt. The major ugliness that you'll notice is a RawId version of getProperty().
Attachment #700148 - Flags: review?(terrence)
For the next patch, I needed HandleId's of EncapsulatedId's. It was a bit more work than I expected.
Attachment #700149 - Flags: review?(terrence)
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.)
Attachment #700148 - Flags: review?(terrence)
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.
Attachment #700149 - Attachment is obsolete: true
Attachment #700149 - Flags: review?(terrence)
Attached patch jsid rootingSplinter Review
Rewritten patch. Now standalone.

why has bzexport forsaken me??!
Attachment #700148 - Attachment is obsolete: true
Attachment #700589 - Flags: review?(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.
Attachment #700589 - Flags: review?(terrence) → review+
Followup fixes to jit-test breakage induced by the other patch.
Attachment #701309 - Flags: review?(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?
Attachment #701309 - Flags: review?(terrence)
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.)
Attachment #701595 - Flags: review?(terrence)
Attachment #701309 - Attachment is obsolete: true
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.
Attachment #701595 - Flags: review?(terrence) → review+
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.)
Attachment #703147 - Flags: review?(terrence)
Attachment #701595 - Attachment is obsolete: true
Whoops, forgot to remove some of the extra Unrooted/DropUnrooted pairs that you mentioned in an earlier review.
Attachment #703379 - Flags: review?(terrence)
Attachment #703147 - Attachment is obsolete: true
Attachment #703147 - Flags: review?(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.
Attachment #703379 - Flags: review?(terrence) → review+
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
Attachment #703535 - Flags: review?(terrence)
Comment on attachment 703535 [details] [diff] [review]
Make JSObject a class instead of a struct

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

Exciting!
Attachment #703535 - Flags: review?(terrence) → review+
Attachment #700589 - Flags: checkin+
Attachment #703379 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/a5f5694ad2c0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.