Closed Bug 713183 Opened 12 years ago Closed 12 years ago

Split all the property-access ops into property, element, special, and by-value ops

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(4 files)

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.
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?
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.
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.
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.
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.
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.
Attached patch WIPSplinter Review
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.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
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.
Attachment #584493 - Flags: review?(bhackett1024)
Attachment #584492 - Flags: review?(bhackett1024) → review+
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.
Attachment #584498 - Flags: review?(bhackett1024)
Attachment #584493 - Flags: review?(bhackett1024) → review+
Attachment #584498 - Flags: review?(bhackett1024) → review+
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e3133d19e2 for the last part.
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/d0e3133d19e2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 736747
You need to log in before you can comment on or make changes to this bug.