Last Comment Bug 713183 - Split all the property-access ops into property, element, special, and by-value ops
: Split all the property-access ops into property, element, special, and by-val...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on: 715682 736747
Blocks: 586842
  Show dependency treegraph
 
Reported: 2011-12-22 22:32 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-03-17 09:45 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (114.81 KB, patch)
2011-12-27 00:30 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Review
Don't generate JSOP_INITPROP for string-but-index properties in object literals (1.39 KB, patch)
2011-12-27 14:40 PST, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Review
Restrict the functionality of the shell's stats() builtin (2.62 KB, patch)
2011-12-27 14:49 PST, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Review
Switch all the JSOP*PROP opcodes to have a PropertyName* instead of a JSAtom* (112.69 KB, patch)
2011-12-27 15:19 PST, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-22 22:32:31 PST
We have property now (the "prop" variants), sort of.  (Sometimes the prop is actually an index, e.g. "4294967295".  But at least the form's in place.)  We have by-value now (the "elem" variants, I think to a tee).  We do not have element (a string that's not a PropertyName, i.e. an unsigned 32-bit integer) now.  We need it to make the element/non-element storage split understandable.  We also don't have special now.  I don't think special is strictly necessary for this.  But I'm not sure of that yet.  (And adding element is going to touch all the code that'd be touched to add special, so I might as well do both while it's all fresh in the mind.)

I was hoping to introduce this split opcode by opcode.  Unfortunately, because there's only one bit of code to parse any property access, I think I'm going to have to add them all at once, kind of.  I think I *might* be able to hook up the actual implementation op by op, by having the emitter do the old-style thing for the new ops I haven't implemented yet.  Maybe.  We'll see.

I have a patch that actually compiles which implements just the delete property/element/by-value ops (it lacks a special variant).  I am 100% certain, without testing, that it doesn't work yet.  But it is a reasonable start.  I'm going to predict that, with concentrated hacking aided by few people being around for awhile, I can finish this bug by year's end, and have at least one element-based op working.  We'll see.
Comment 1 Brian Hackett (:bhackett) 2011-12-23 07:03:29 PST
What is the definitely-element variant for?  Can we just have a *PROP variant and a *ELEM variant (as we do now) and just ensure that the *PROP is taking a PropertyName, and allow *ELEM to take an arbitrary value?
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-23 11:35:53 PST
Definitely-element says you don't have to examine the value to determine whether it's a uint32_t -- it just is.  The opcode can be implemented by obj->getElement(...).  The property variant would be implemented by obj->getProperty(...).  (And special by getSpecial(...).)  With the arbitrary-value opcode, at least some examination is necessary to determine which property variant is being specified.
Comment 3 Brian Hackett (:bhackett) 2011-12-23 11:50:07 PST
Yes, but the parser will only know an accessed value is a uint32 when it is a constant.  The JITs already know how to optimize accesses on constant indexes, and this change wouldn't make things simpler, because the JIT will do constant folding first and may need to handle constants at the generic versions of the opcodes too.  Introducing all this code and complexity so the interpreter can sometimes shave a couple cycles makes no sense at all.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-23 12:23:19 PST
This isn't so the interpreter can shave a few cycles, it's so that I'm sure I've made sense of how everything fits together internally, so I'm not introducing type errors by assuming a JSAtom* is a PropertyName* when really it's not.  I've introduced type errors in the past by making this mistake; pushing the distinction into bytecode ensures it's impossible to make that mistake again.  This is tricky business to get right, given how long we've used JSAtom* as any arbitrary string.  I am not confident in my ability to do it correctly without the type-safety assurances of more-constrained bytecodes.
Comment 5 Brian Hackett (:bhackett) 2011-12-23 12:45:03 PST
OK, but I don't see how the approach in comment 1 doesn't address this problem.  The parser can ensure that all *PROP opcodes are working on PropertyNames, and you should be able to enforce this in the type system by changing places where *PROP opcods are used to fetch a PropertyName instead of a JSAtom, and change the parser emit functions that work on properties to take a PropertyName.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-26 17:55:56 PST
Hmm.  I'm not so sure it'll be that easy, but it might work.  There's enough flow of atoms that I'm worried how this'll work without specifically adding an element path to be sure every edge is smoked out.  But I can run with that tack for a bit to see.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-27 00:30:09 PST
Created attachment 584398 [details] [diff] [review]
WIP

This compiles and runs enough to do a little bit of triviality.  I'm aware of at least two bugs in it, one in js.cpp:DumpStats, and one with this snippet, because object initializers aren't property/non-property split yet:

js> for (var i = 0; i < 10; i++) (function() { return ({ x: 5, "4294967286": 3 }) })()
Assertion failure: !isIndex(&dummy), at ../../vm/String.h:854
Aborted (core dumped)

There are probably more.  But it's a start.

This is atop a bunch of other in-progress patches, so it probably doesn't easily apply to much outside my tree.
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-27 14:40:06 PST
Created attachment 584492 [details] [diff] [review]
Don't generate JSOP_INITPROP for string-but-index properties in object literals
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-27 14:49:32 PST
Created attachment 584493 [details] [diff] [review]
Restrict the functionality of the shell's stats() builtin

stats() tries to find a property on the scope chain (?) without verifying that the property name is actually a string first.  This is all kinds of dodgy, and it violates some type changes the actual patch here wants to make (to use PropertyName* for some types rather than jsid).  Plus, it doesn't really even work anyway --it just crashes for me if I use a proper property name and associate an object value with it.  So let's just remove this broken functionality.  (It seems to have existed since the original Netscape source code release, if you track far enough back.  There's no explanation for its raison d'être that I can find.)  Tests pass with this removal.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-27 15:19:06 PST
Created attachment 584498 [details] [diff] [review]
Switch all the JSOP*PROP opcodes to have a PropertyName* instead of a JSAtom*

This is type-safe only in the sense that all accesses to the PropertyName* immediate will go through a type-check, via asPropertyName().  The bytecode emitter is utterly ignorant of name, and it still keeps doing things with pn_atom.  There's a lot more abstraction-keeping that could, and should, be done here for believability.

Having two index-ness concepts floating around for awhile (js_CheckForStringIndex and uint32_t-style indexness) is going to be awful.  I'm going to move pretty quickly on switching storage formats after this lands, so that we can get rid of the former sense as early as possible.  My initial patching (once I had a compiling patch) broke stuff almost entirely because I didn't consider that the two were still distinct with this change.
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-29 01:01:00 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba18c8c2e6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4193185e67d

The third patch still needs to land, so this shouldn't be closed with the next m-c merge just yet.  That patch depends on a mess of parser patches jorendorff's reviewing, and it would be painful to disentangle those dependencies to land it earlier.
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-03 20:34:30 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e3133d19e2 for the last part.
Comment 14 Marco Bonardo [::mak] 2012-01-04 04:53:26 PST
https://hg.mozilla.org/mozilla-central/rev/d0e3133d19e2

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