Comment 1 Nicholas Nethercote [:njn] 2011-07-06 17:34:35 PDT

Comment on attachment 544371 [details] [diff] [review]
upvarMap as vector.

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

This seems fine, but the point is unclear.  I guess it's good to replace an ad-hoc array type with js::Vector?  But there's still several other JSUpvarArrays around;  getting rid of all of them would be a bigger win.

::: js/src/jsemit.cpp
@@ +2341,5 @@
> +            size_t lexdepCount = cg->roLexdeps->count();
> +
> +            JS_ASSERT_IF(!upvarMap.empty(), lexdepCount == upvarMap.length());
> +            if (upvarMap.empty())
> +                upvarMap.appendN(UpvarCookie(), lexdepCount);

Probably better to use infallibleAppendN(), it makes the precondition clearer.

Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2011-07-06 17:59:44 PDT

(In reply to comment #1)
> This seems fine, but the point is unclear.  I guess it's good to replace an
> ad-hoc array type with js::Vector?  But there's still several other
> JSUpvarArrays around;  getting rid of all of them would be a bigger win.

The other ones are uses of the type embedded in JSScript, and we hate increasing the size of JSScript for memory footprint reasons.

Second comment is good -- it's actually fallible and I forgot to check! It's just a lazy vector capacity initialization like in the original.

Comment 3 Chris Leary [:cdleary] (not checking bugmail) 2011-07-06 18:25:13 PDT

http://hg.mozilla.org/tracemonkey/rev/1013f4be025f

Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-13 18:27:37 PDT

Backed out of m-i as part of a mass backout.

Comment 5 :Ehsan Akhgari 2011-07-15 06:57:41 PDT

http://hg.mozilla.org/mozilla-central/rev/2bdcc80c8324