657665 - ArgumentsObject needs documentation, fast-path methods to access element values

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: 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 [Laid off!]]

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]]

Attachment: 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 [Laid off!]]

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 [Laid off!]]

(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]]

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)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d52f353139fc

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d52f353139fc

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d52f353139fc