669786 - JSCodeGenerator::upvarMap should be a js::Vector

Status: RESOLVED FIXED

Description

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

Created attachment 544371 [details] [diff] [review]
upvarMap as vector.

Spotted this when I was working on another bug.

Comment 1

[Nicholas Nethercote [:njn]]

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

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

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

Comment 4

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

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

Comment 5

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

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