Closed
Bug 828753
Opened 12 years ago
Closed 12 years ago
jsid rooting, mostly in jsinfer.*
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(3 files, 5 obsolete files)
(deleted),
patch
|
terrence
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
This is my first attempt. The major ugliness that you'll notice is a RawId version of getProperty().
Attachment #700148 -
Flags: review?(terrence)
Assignee | ||
Comment 2•12 years ago
|
||
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)
Assignee | ||
Comment 3•12 years ago
|
||
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)
Assignee | ||
Comment 4•12 years ago
|
||
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)
Assignee | ||
Comment 5•12 years ago
|
||
Rewritten patch. Now standalone.
why has bzexport forsaken me??!
Attachment #700148 -
Attachment is obsolete: true
Attachment #700589 -
Flags: review?(terrence)
Updated•12 years ago
|
Blocks: ExactRooting
Comment 6•12 years ago
|
||
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+
Assignee | ||
Comment 7•12 years ago
|
||
Followup fixes to jit-test breakage induced by the other patch.
Attachment #701309 -
Flags: review?(terrence)
Comment 8•12 years ago
|
||
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)
Assignee | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #701309 -
Attachment is obsolete: true
Comment 10•12 years ago
|
||
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+
Assignee | ||
Comment 11•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #701595 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
Whoops, forgot to remove some of the extra Unrooted/DropUnrooted pairs that you mentioned in an earlier review.
Attachment #703379 -
Flags: review?(terrence)
Assignee | ||
Updated•12 years ago
|
Attachment #703147 -
Attachment is obsolete: true
Attachment #703147 -
Flags: review?(terrence)
Comment 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
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 15•12 years ago
|
||
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+
Assignee | ||
Updated•12 years ago
|
Attachment #700589 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #703379 -
Flags: checkin+
Assignee | ||
Comment 16•12 years ago
|
||
Combined patches pushed as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fce4e0f8a553
Comment 17•12 years ago
|
||
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.
Description
•