If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

-memstats periodically hanging on mac

VERIFIED FIXED in flash10.1

Status

Tamarin
Garbage Collection (mmGC)
P2
major
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Brent Baker, Assigned: Erik Tierney)

Tracking

unspecified
flash10.1
x86
Mac OS X
Bug Flags:
flashplayer-qrb +
flashplayer-triage +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
I am seeing periodic hangs when using -memstats and running the sunspider/string-unpack-code.as test.

I have reproduced the issue on 3 different 10.4 machines (intel and PPC), but have not been able to reproduce on a 10.5 machine.

The hang happens in both release and release_debugger builds but not in the debug builds.
Flags: flashplayer-triage?
Flags: flashplayer-qrb?
(Reporter)

Updated

9 years ago
Blocks: 478870

Updated

9 years ago
Assignee: nobody → treilly
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → flash10.x

Updated

9 years ago
Assignee: treilly → tierney

Updated

9 years ago
Flags: flashplayer-triage?
Flags: flashplayer-triage+
Flags: flashplayer-qrb?
Flags: flashplayer-qrb+
(Assignee)

Comment 1

9 years ago
Created attachment 367282 [details] [diff] [review]
Fix memstats hang on mac

Memstats was hanging because the blocks were getting screwed up.  The code to merge blocks was wrong, and was merging to blocks together that were not contiguous.  Changed the code to make sure the two blocks were contiguous before merging them.  

Also added some ifdef'ed out debugging code to look for errors in blocks/regions.
Attachment #367282 - Flags: review?(treilly)

Comment 2

9 years ago
Comment on attachment 367282 [details] [diff] [review]
Fix memstats hang on mac

I don't think this is right, non-contiguous heap sections should be separated with a sentinel block.  This is old code that hasn't changed in awhile, something else changed recently that introduced this hang

Updated

9 years ago
Attachment #367282 - Flags: review?(treilly) → review-
(Assignee)

Comment 3

9 years ago
Created attachment 367624 [details] [diff] [review]
Add in sentinel when removing blocks

Change RemoveBlock to add in a sentinel since any block removed from the middle will mean the prev and next blocks are not contiguous.
Attachment #367282 - Attachment is obsolete: true
Attachment #367624 - Flags: review?(treilly)

Updated

9 years ago
Attachment #367624 - Flags: review?(treilly) → review+
(Assignee)

Comment 4

9 years ago
pushed:
http://hg.mozilla.org/tamarin-redux/rev/de7b873a09a7
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 5

9 years ago
Created attachment 367784 [details] [diff] [review]
Turn memstats fix back on, with fixes

This fixes the problems we were having with my previous fix.  Now it only inserts a sentinel if the block is removed from the middle of a contiguous region.  If the block is not contigous with either the previous or next block then there will already be a sentinel.  If the block is not contigous with both the previous and next block, we now remove a sentinel, since there would have been 2 (one on either side).  

Also patches up some bad math when we were redoing the region->blockIds and the prev/next pointers for the blocks that got moved.
Attachment #367624 - Attachment is obsolete: true
Attachment #367784 - Flags: review?(treilly)

Comment 6

9 years ago
Comment on attachment 367784 [details] [diff] [review]
Turn memstats fix back on, with fixes

There's a redundant setting of need_sentinel to false but otherwise looks good
Attachment #367784 - Flags: review?(treilly) → review+
(Assignee)

Comment 7

9 years ago
pushed:
http://hg.mozilla.org/tamarin-redux/rev/cbeb92c4f2aa
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 8

8 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.