Last Comment Bug 669786 - JSCodeGenerator::upvarMap should be a js::Vector
: JSCodeGenerator::upvarMap should be a js::Vector
Status: RESOLVED FIXED
[fixed-in-tracemonkey]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 670772
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-06 16:37 PDT by Chris Leary [:cdleary] (not checking bugmail)
Modified: 2011-07-15 06:57 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
upvarMap as vector. (7.04 KB, patch)
2011-07-06 16:37 PDT, Chris Leary [:cdleary] (not checking bugmail)
n.nethercote: review+
Details | Diff | Splinter Review

Description Chris Leary [:cdleary] (not checking bugmail) 2011-07-06 16:37:56 PDT
Created attachment 544371 [details] [diff] [review]
upvarMap as vector.

Spotted this when I was working on another bug.
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.

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