Closed Bug 828753 Opened 12 years ago Closed 12 years ago

jsid rooting, mostly in jsinfer.*

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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.
Attached patch jsid rooting, mostly in jsinfer.* (obsolete) (deleted) — Splinter Review
This is my first attempt. The major ugliness that you'll notice is a RawId version of getProperty().
Attachment #700148 - Flags: review?(terrence)
Attached patch create HandleT from EncapsulatedT (obsolete) (deleted) — Splinter Review
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 rooting (deleted) — Splinter Review
Rewritten patch. Now standalone. why has bzexport forsaken me??!
Attachment #700148 - Attachment is obsolete: true
Attachment #700589 - Flags: review?(terrence)
Blocks: ExactRooting
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+
Attached patch Misc rooting fixes exposed by the jsid patch (obsolete) (deleted) — Splinter 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)
Attached patch Misc rooting fixes exposed by the jsid patch (obsolete) (deleted) — Splinter Review
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+
Attached patch Misc rooting fixes exposed by the jsid patch (obsolete) (deleted) — Splinter 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+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: