Last Comment Bug 657665 - ArgumentsObject needs documentation, fast-path methods to access element values
: ArgumentsObject needs documentation, fast-path methods to access element values
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla6
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks: 656813
  Show dependency treegraph
 
Reported: 2011-05-17 09:34 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-05-23 14:08 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (10.19 KB, patch)
2011-05-17 09:34 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review
Significantly more correct patch, with tests (12.62 KB, patch)
2011-05-20 15:15 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review

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

Docs are good.  And bug 656813 wants those methods (which exist but not as methods, spread across various locations now).
Comment 1 Brian Hackett (:bhackett) 2011-05-17 12:08:04 PDT
Comment on attachment 532994 [details] [diff] [review]
Patch

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

Looks good, r+ with changes.

::: js/src/vm/ArgumentsObject.h
@@ +125,5 @@
> + *   }
> + *   f("arga", "argb");
> + *
> + * ES5's strict mode behaves more sanely and makes named arguments not alias
> + * arguments provided to the function call.

Maybe 'not alias elements of the arguments object' ?

@@ +220,5 @@
> +     * loop.
> +     *
> +     * NB: Returning false does not indicate error!
> +     */
> +    inline bool getElements(uint32 start, uint32 end, js::Value *vp);

I think that getElement should be folded into getElements.  Having two implementations with similar functionality, in two different files and implemented in slightly different ways is sort of confusing.  getElements can have a fast path up front for (end == start + 1), or maybe use 'count' instead of 'end' to make it easier for the compiler to optimize when inlining.

JSObject::ensureDenseArrayElements is a similar case that cleanly and efficiently handles any input.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-20 15:15:10 PDT
Created attachment 534126 [details] [diff] [review]
Significantly more correct patch, with tests

The getElements code was completely wrong before.  This fixes it, with tests demonstrating correctness.

Given the mass of complexity I had to work through in forEachCanonicalActualArg, I'm not convinced it's a good idea to unify these.  I would still prefer to have two separate methods, even if they're slightly repetitive.  (I moved both into Stack-inl.h so they're at least adjacent.)
Comment 3 Brian Hackett (:bhackett) 2011-05-20 17:33:23 PDT
Comment on attachment 534126 [details] [diff] [review]
Significantly more correct patch, with tests

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

Ah, read the code closer this time (reminds me why I like writing/using tools to look for buffer overflows).  r+ with fixes.

::: js/src/vm/Stack-inl.h
@@ +516,4 @@
>  
>  template <class Op>
>  inline bool
> +StackFrame::forEachCanonicalActualArg(Op op, uintN start /* = 0 */, uintN count /* = uintN(-1) */)

This function is pretty hard to follow along with now, I think reducing the number of variables in use (suggestions below) would help.

@@ +529,5 @@
> +    JS_ASSERT(start + count <= nactual);
> +
> +    if (start + count <= nformal) {
> +        Value *p = formals + start;
> +        for (Value *end = p + count; p < end; ++p, ++start) {

If you move the 'end = start + count' out of the 'else' branch below and into the header (removing two uses of 'start + count' in the process), you should be able to make the loop test 'start < end' and avoid the 'Value *end'.

@@ +538,3 @@
>          Value *formalsEnd = formalArgsEnd();
> +        uintN end = start + count;
> +        for (Value *p = formals + start; p < formalsEnd; ++p, ++start) {

You should be able to remove the reference to formalsEnd here by making the loop test 'start < nformal', though you'll need formalsEnd below anyways.

@@ +543,5 @@
>          }
> +        JS_ASSERT(start >= nformal);
> +        Value *actuals = formalsEnd - (nactual + 2) + (start - nformal);
> +        Value *actualsEnd = formals - 2 - (nactual - end);
> +        for (Value *p = actuals; p < actualsEnd; ++p, ++start) {

You should be able to get rid of actualsEnd here by making the loop test 'start < end'.

@@ +1106,5 @@
> +
> +inline bool
> +ArgumentsObject::getElement(uint32 i, Value *vp)
> +{
> +    *vp = element(i);

Minor thing, but it would be nice if getElement(i, vp) had the same contract as getElements(i, 1, vp), which should be the case if getElement did its own bounds checking and didn't require every caller to check range against initialLength.

@@ +1140,5 @@
> +{
> +    JS_ASSERT(start + count >= start);
> +
> +    uint32 length = initialLength();
> +    if (start > length || start + count > length)

This test permits args where 'start == length' and 'count == 0', and will I think bust the 'start + count <= nactual' assert in forEachCanonicalActualArg.  Should use '>=' here instead.
Comment 4 Brian Hackett (:bhackett) 2011-05-21 07:35:32 PDT
(In reply to comment #3)
> @@ +1140,5 @@
> > +{
> > +    JS_ASSERT(start + count >= start);
> > +
> > +    uint32 length = initialLength();
> > +    if (start > length || start + count > length)
> 
> This test permits args where 'start == length' and 'count == 0', and will I
> think bust the 'start + count <= nactual' assert in
> forEachCanonicalActualArg.  Should use '>=' here instead.

Oops, nevermind on this comment.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-22 21:12:05 PDT
http://hg.mozilla.org/tracemonkey/rev/d52f353139fc
http://hg.mozilla.org/tracemonkey/rev/d1a9c3456499
http://hg.mozilla.org/tracemonkey/rev/2053eec43ed6

Somehow I managed to not add the test to the patch posted here -- feel free to take a look if you want (it's in the first revision), also would have demonstrated that the last concern in comment 3 was a definite misread.  Given the mess of complexity there I wouldn't have pushed ahead on this without that test (and delayed posting a new patch until I found time to write that test).
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2011-05-23 01:13:09 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d52f353139fc
Comment 7 Chris Leary [:cdleary] (not checking bugmail) 2011-05-23 03:21:34 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d52f353139fc
Comment 8 Chris Leary [:cdleary] (not checking bugmail) 2011-05-23 14:08:12 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d52f353139fc

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