Last Comment Bug 772688 - add BindingIter and use it instead of directly touching a Binding's shape
: add BindingIter and use it instead of directly touching a Binding's shape
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 753158
Blocks: 767013
  Show dependency treegraph
 
Reported: 2012-07-10 16:32 PDT by Luke Wagner [:luke]
Modified: 2012-07-18 05:55 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (36.47 KB, patch)
2012-07-10 16:32 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
patch (38.20 KB, patch)
2012-07-10 16:34 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review
rm CallObject::setVarOp (12.20 KB, patch)
2012-07-10 19:33 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-07-10 16:32:33 PDT
Created attachment 640852 [details] [diff] [review]
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] 2012-07-10 16:34:39 PDT
Created attachment 640853 [details] [diff] [review]
patch

oops, qref'd
Comment 2 Luke Wagner [:luke] 2012-07-10 19:33:32 PDT
Created attachment 640897 [details] [diff] [review]
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] 2012-07-10 19:53:15 PDT
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 (:bhackett) 2012-07-13 09:26:13 PDT
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] (remove +bmo to email) 2012-07-16 10:15:07 PDT
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] 2012-07-17 11:44:14 PDT
(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

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