772688 - add BindingIter and use it instead of directly touching a Binding's shape

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

In preparation for bug 767013, we need to stop touching the shapes of Bindings directly and instead iterate over some abstract sequence (so that we can stop creating Shapes for unaliased variables).

In the process, I was able to remove the last CallObject::set(Var|Arg)Op JSPropertyOps, which is a nice bonus.

(Note: this patch sits atop bug 753158.)

Comment 1

[Luke Wagner [:luke]]

Attachment: patch

oops, qref'd

Comment 2

[Luke Wagner [:luke]]

Attachment: rm CallObject::setVarOp

This patch actually removes set(Var|Arg)Op.  (This is based on bug 753158, which is why the barriers shouldn't be necessary.)

Comment 3

[Luke Wagner [:luke]]

On a side note, it wouldn't be hard to remove CallObject/StaticBlockObject's use of shortids now.  That leaves only the "tinyid" use made in JSAPI.  A tinyid is only 8 bits so maybe we could stuff it in the BaseShape::flags?  Ideally we'd just remove tinyids, but there seem to be a few browser uses, so I don't know how hard that is.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 640897 [details] [diff] [review]
rm CallObject::setVarOp

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

::: js/src/methodjit/PolyIC.cpp
@@ -347,5 @@
> -            // >--+--<      of 'fun' remaining the same. However, since:
> -            //   |||         1. the shape includes all arguments and locals and their setters
> -            //    \\     V     and getters, and
> -            //      \===/    2. arguments and locals have different getters
> -            //              then we can rely on fun->nargs remaining invariant.

Oh no, PolyIC.cpp loses its mascot.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 640853 [details] [diff] [review]
patch

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

::: js/src/jit-test/tests/arguments/strict-args-flushstack.js
@@ +11,5 @@
>    var a = [];
>    for (var i = 0; i < 9; i++)
>      a.push(arguments);
> +  print("args: " + args);
> +  print("a[0]: " + a[0]);

Debugging detritus definitely deserves deletion.

::: js/src/jsscript.cpp
@@ +51,5 @@
>  using namespace js::gc;
>  using namespace js::frontend;
>  
> +BindingIter::BindingIter(const Bindings &bindings, Shape *shape)
> + : bindings_(&bindings), shape_(shape)

Needs another space of indentation.

@@ +57,5 @@
> +    settle();
> +}
> +
> +BindingIter::BindingIter(Bindings &bindings)
> + : bindings_(&bindings), shape_(bindings.lastBinding)

Another space.

::: js/src/jsscript.h
@@ +25,5 @@
> +struct Shape;
> +
> +namespace mjit {
> +    struct JITScript;
> +    class CallCompiler;

I haven't seen anyone indenting the contents of namespaces, even for declaring things.  I guess this was pre-existing; undo?

@@ +101,5 @@
> +{
> +    friend class Bindings;
> +    BindingIter(const Bindings &bindings, Shape *shape);
> +    void settle();
> +  public:

Blank line before this.

Comment 6

[Luke Wagner [:luke]]

(In reply to Jeff Walden [:Waldo] (gone July 18-August 26) from comment #5)
> Debugging detritus definitely deserves deletion.

done

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e8d41e9f8ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/0be7b0709e5d

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0be7b0709e5d
https://hg.mozilla.org/mozilla-central/rev/2e8d41e9f8ef