Last Comment Bug 670772 - JSCodeGenerator::upvarMap should use exactly as much space as it needs
: JSCodeGenerator::upvarMap should use exactly as much space as it needs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla8
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
Mentors:
Depends on:
Blocks: 669786
  Show dependency treegraph
 
Reported: 2011-07-11 15:21 PDT by Chris Leary [:cdleary] (not checking bugmail)
Modified: 2011-07-21 22:47 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allocate less for upvarMap. (2.77 KB, patch)
2011-07-11 15:21 PDT, Chris Leary [:cdleary] (not checking bugmail)
n.nethercote: review+
Details | Diff | Review

Description Chris Leary [:cdleary] (not checking bugmail) 2011-07-11 15:21:34 PDT
Created attachment 545268 [details] [diff] [review]
Allocate less for upvarMap.

appendN was causing us to over-allocate. I also took a look at the number of upvar cookies we typically allocated on parsemark and they have low averages, so I added some inline storage.

http://groups.google.com/group/mozilla.dev.tree-management.tracemonkey/browse_thread/thread/e4d6c348965025f9#
Comment 1 Nicholas Nethercote [:njn] 2011-07-12 19:12:09 PDT
Comment on attachment 545268 [details] [diff] [review]
Allocate less for upvarMap.

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

Looks plausible.  But I really want to see the memory regression disappear!  Can you report back to confirm that it's happened?  If it doesn't fix the regression, I think we should back both this and bug 669786 out.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-07-13 18:27:49 PDT
Backed out of m-i as part of a mass backout.
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2011-07-15 06:58:00 PDT
http://hg.mozilla.org/mozilla-central/rev/3cd0a17b27bf
Comment 4 David Mandelin [:dmandelin] 2011-07-15 13:59:10 PDT
It looks like the regression was fixed:

http://graphs.mozilla.org/graph.html#tests=[[29,4,219]]&sel=1305850194,1310436088

Am I reading that right?
Comment 5 Chris Leary [:cdleary] (not checking bugmail) 2011-07-18 11:46:04 PDT
(In reply to comment #4)
> Am I reading that right?

I never landed the followup patch on tracemonkey, so I'm skeptical.
Comment 6 Nicholas Nethercote [:njn] 2011-07-21 21:04:59 PDT
If we don't get clear evidence that this fixed the original regression by early next week, I'm going to back out this patch and the previous one.  I don't want to accept a regression due to time passing and everyone forgetting about it because the bug was marked FIXED.
Comment 7 Chris Leary [:cdleary] (not checking bugmail) 2011-07-21 22:05:09 PDT
(In reply to comment #6)

We never saw a regression report on mozilla-central, so the two patch combo didn't regress anything. TM was frozen after the first one landed and before the two-patch combo went to inbound.
Comment 8 Nicholas Nethercote [:njn] 2011-07-21 22:27:16 PDT
(In reply to comment #7)
> (In reply to comment #6)
> 
> We never saw a regression report on mozilla-central, so the two patch combo
> didn't regress anything.

So the two patches landed separately on mozilla-central, and we never saw a regression?  That makes it sound like the TM regression was bogus, though it's odd that it seemingly lasted for a few pushes before going away.  Huh.  Ok, I'm satisfied.
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2011-07-21 22:47:47 PDT
(In reply to comment #8)
> Ok, I'm satisfied.

Yay! We potentially malloc less stuff now because of the inline vector size for typical upvarmaps, so that makes me happy.

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