Last Comment Bug 770344 - Experiment implementing __proto__ as an accessor
: Experiment implementing __proto__ as an accessor
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
: 699945 (view as bug list)
Depends on: 770586 773850
  Show dependency treegraph
Reported: 2012-07-02 14:44 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2013-02-13 15:26 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (25.70 KB, patch)
2012-07-02 14:44 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
dmandelin: feedback+
Details | Diff | Splinter Review
Ready for full review, passes try (27.73 KB, patch)
2012-07-13 23:36 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-02 14:44:34 PDT
Created attachment 638505 [details] [diff] [review]

Other engines are testing this out.  We should, too; it's fairly simple to do, and implementing it this way has several nice properties that implementing it the current way doesn't.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-02 15:03:13 PDT
Comment on attachment 638505 [details] [diff] [review]

dmandelin, feel free to offer any comments you have on this, too, since I guess this is module-owner-ish territory.
Comment 2 Luke Wagner [:luke] 2012-07-02 15:54:03 PDT
Comment on attachment 638505 [details] [diff] [review]

I like the patch; it moves the engine one key step away from JSPropertyOp which, it seems, we should ultimately remove.  jsc has already done this which would imply that the flip to accessor property doesn't break the web.

One nit: I'd like the getter/setter to use BoxNonStrictThis (after error-checking null/undefined) instead of explicitly handling primitive types.

(The title says "experimental", so I assume this won't be landing soon without some higher-level semantic consensus.)
Comment 3 David Mandelin [:dmandelin] 2012-07-09 18:00:45 PDT
Comment on attachment 638505 [details] [diff] [review]

Review of attachment 638505 [details] [diff] [review]:

I like the idea of moving the implementation away from JSPropertySpec getters/setters.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-13 23:36:48 PDT
Created attachment 642181 [details] [diff] [review]
Ready for full review, passes try

Okay, this is good to go, and there are no lingering issues which have me suspicious that it might possibly have any lurking fundamental problems.

If you want to test this, apply the patch in bug 773850, then apply this atop it.  I pushed a very-slightly-different version of this patch to try and got only unrelated M4 failures, so the concept is solid.  Here's a repush of this exact patch to hopefully get a fully clean bill of health:

As you're aware, speedy review here and in bug 773850 is highly appreciated, given my impending travel plans.  :-)
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-14 11:22:25 PDT
The X orange in that try-push is from my changes being atop a bad m-i changeset, which was later backed out for it:

So that's a definite clean bill of health.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-17 09:20:08 PDT
Since we're at the very start of a development cycle, I pushed to get data as to the effectiveness of this technique.  There's no substitute for good, hard data.  And we can always back it out if necessary, or tweak if we hit easily-fixt issues.
Comment 7 Ed Morley [:emorley] 2012-07-18 05:57:05 PDT
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-13 15:26:29 PST
*** Bug 699945 has been marked as a duplicate of this bug. ***

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