JSCodeGenerator::upvarMap should use exactly as much space as it needs

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cdleary, Assigned: cdleary)

Tracking

unspecified
mozilla8
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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#
Attachment #545268 - Flags: review?(nnethercote)
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.
Attachment #545268 - Flags: review?(nnethercote) → review+
Whiteboard: [inbound]
Backed out of m-i as part of a mass backout.
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/3cd0a17b27bf
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
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?
(In reply to comment #4)
> Am I reading that right?

I never landed the followup patch on tracemonkey, so I'm skeptical.
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.
(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.
(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.
(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.
You need to log in before you can comment on or make changes to this bug.