mmgc intermittent assert running spidermonkey performance test

VERIFIED FIXED in flash10.1


9 years ago
9 years ago


(Reporter: cpeyer, Assigned: lhansen)


Windows XP
Bug Flags:
in-testsuite +
flashplayer-qrb +
flashplayer-triage +


(Whiteboard: Has patch, skipped-in-testconfig)


(4 attachments)



9 years ago
Created attachment 432016 [details]

Assertion failed: "(((amountRecommitted > 0)))" ("/tamarin-redux/windows/repo/MMgc/GCHeap.cpp":1123)
avmplus crash: exception 0x80000003 occurred
Writing minidump crash log to avmplusCrash.dmp

Unfortunately I'm unable to isolate the issue into a smaller individual testcase outside of our framework.  I reproduce the assert about 30% of the time of my machine.

Test: spidermonkey/js1_5/String/

Only happens on win and win64.
Flags: in-testsuite+
Flags: flashplayer-triage+
Flags: flashplayer-qrb?

Comment 1

9 years ago
Created attachment 432017 [details]
abc that causes intermittent assert

Comment 2

9 years ago
Created attachment 432018 [details]
Testcase from acceptance suite.


9 years ago
Whiteboard: skipped-in-testconfig


9 years ago
Assignee: nobody → lhansen
Flags: flashplayer-qrb? → flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.1


9 years ago
Attachment #432018 - Attachment mime type: application/octet-stream → text/plain

Comment 3

9 years ago
Analysis so far: 

The assertion is incorrect, but there are also other bugs.

The AllocBlock function takes a request and tries to find a block that satisifies that request.  It will always prefer a single, committed block to any combination of coalesced blocks.  If a single committed block cannot be found then an uncommitted block /must/ be found in order to trigger a coalescing search.  That would be correct /if/ committed blocks were always coalesced; however, so far as I can tell, they are not reliably coalesced.

The first bug is that an uncommitted block is required to start a search, but such a block may not exist on the free list.  Ergo some storage may languish on the free list for some time.

A coalescing search starts with an uncommitted block and scans backward for free blocks that can be added to it, stopping when the accumulated block meets the request size or the search has reached the beginning of the list.  Then a forward search may also be triggered.

The second bug is that the forward search is triggered unless the accumulated size is /greater/ than the request (not greater or equal).  This is not a big deal, but it's wrong.

Once the search has completed we have a reference to the first coalescable block in a list of such blocks.  We now coalesce blocks until the request has been met, and count the amount of memory we commit in the process.

The third bug, which is why the assert fires, is that the coalescing process may touch only committed blocks.  Consider a request for 20 in a list containing 10,10,1 where the last is uncommitted and the others are committed.  The backward search will return a reference to the first 10 block; those two will be coalesced and the resulting block of 20 returned, but the amountRecommitted will be zero.  Again the underlying problem here is that committed blocks are not always coalesced.

Comment 4

9 years ago
Another bug touching on free list management is bug #523582.

Comment 5

9 years ago
blocks are supposed to be eagerly coalesced, that's the root problem

Comment 6

9 years ago
Another possibility is that the 10 adjacent contiguous blocks aren't actually contiguous and we have a bug in RemoveBlock where it isn't inserting a sentinel properly to keep non-contiguous blocks separate.

Comment 7

9 years ago
Created attachment 432962 [details] [diff] [review]
Assertions, bug fixes

Contains the following substantive changes:

- Adds assertions in CheckFreeList to test that no block on the free list has a neighbor on the free list with the same committed status.  Essentially the assertions check that eager coalescing always works.

- Moves the coalescing code from FreeBlock into AddToFreeList, so every add of a committed block will look for coalescing opportunities.  Initially I had this as a special-case function but the assertions kept triggering and the code paths are not long, so I figure it's best always to do this.  With this change, two of the three bugs reported in the analysis go away.

- Adds sundry other assertions where they seemed useful.

- Changes the comparison in AllocBlock to use ">=" rather than ">" as the cutoff.
Attachment #432962 - Flags: review?(treilly)


9 years ago
Whiteboard: skipped-in-testconfig → Has patch, skipped-in-testconfig


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

Comment 8

9 years ago
tamarin-redux-argo changeset:   3838:dd364ce52291
Last Resolved: 9 years ago
Resolution: --- → FIXED


9 years ago
Depends on: 554890

Comment 9

9 years ago
Chris can you confirm that this is fixed, looks like the referenced testcase is still marked as skip in the testconfig so it currently is not being run.

Comment 10

9 years ago
test removed from testconfig, pushed to argo changeset 3915	7e5993aa597a
You need to log in before you can comment on or make changes to this bug.