Last Comment Bug 655907 - Start homing class-prototype lookups directly in the global object, not in a windy series of steps beneath js_GetClassPrototype
: Start homing class-prototype lookups directly in the global object, not in a ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 655192 656043
Blocks: 655921
  Show dependency treegraph
 
Reported: 2011-05-09 17:27 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-10-20 03:06 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (27.28 KB, patch)
2011-05-09 17:27 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Don't change js::FindProto (26.22 KB, patch)
2011-05-11 16:50 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-09 17:27:02 PDT
Created attachment 531212 [details] [diff] [review]
Patch

js_GetClassPrototype is overly complex.  It can more or less be replaced with direct accessor methods on the global object that return the value from the appropriate slot.  Get those methods in place, and use them in those places where it's easy to do so.  (That's not all places, currently, because some code and APIs assume dynamic-scoping-like functionality that looks at the call stack to figure it out.  And the call stack is a lie -- bug 631135 -- in some cases, so making it new-style while still preserving old-style misbehavior until a full fix is entertained is somewhat tricky.  For now I'm leaving all such places alone, if the NULL parent passed to js_GetClassPrototype couldn't easily be replaced a fp->scopeChain() or similar.)

Some of these replacements doubtless could use infallible methods because if they're called, they *know* they've been called after the relevant class has been created, or whatever.  But the case-by-case analysis needed to get that right each time an optimization appears possible seems error-prone.  Further, it's very little overhead to check a slot value for being |undefined| or not.  So for now I'm not worrying about it.  When standard classes come with the global object at construction time, then we can switch every use to be infallible.

This builds on the patch stack in bug 655192, of course.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-11 16:50:47 PDT
Created attachment 531802 [details] [diff] [review]
Don't change js::FindProto

Oops, shouldn't have changed FindProto, that's obviously not possible given the parent passed to it could be NULL.
Comment 2 Jason Orendorff [:jorendorff] 2011-07-01 17:11:09 PDT
Comment on attachment 531802 [details] [diff] [review]
Don't change js::FindProto

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

I think it's a little more code total with the patch than without it, but I am going to enjoy writing getOrCreateFunctionPrototype() from now on, instead of searching the codebase for code to crib. r=me.

::: js/src/jsiter.cpp
@@ +1452,5 @@
>  {
>      /* Create and initialize Iterator.prototype. */
> +    JSObject *objectProto = global->getOrCreateObjectPrototype(cx);
> +    if (!objectProto)
> +        return NULL;

return false; here.

::: js/src/methodjit/Compiler.cpp
@@ -3242,5 @@
> -    /*
> -     * Bake in String.prototype. This is safe because of compileAndGo.
> -     * We must pass an explicit scope chain only because JSD calls into
> -     * here via the recompiler with a dummy context, and we need to use
> -     * the global object for the script we are now compiling.

Good deleting.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-19 14:11:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdd89e099e60
Comment 4 Marco Bonardo [::mak] 2011-10-20 03:06:21 PDT
https://hg.mozilla.org/mozilla-central/rev/bdd89e099e60

Note You need to log in before you can comment on or make changes to this bug.