Closed Bug 551840 Opened 10 years ago Closed 10 years ago

mmgc intermittent assert running spidermonkey performance test

Categories

(Tamarin Graveyard :: Virtual Machine, defect, P2)

x86
Windows XP
defect

Tracking

(Not tracked)

VERIFIED FIXED
flash10.1

People

(Reporter: cpeyer, Assigned: lhansen)

References

Details

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

Attachments

(4 files)

Attached file avmplusCrash.dmp
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/regress-157334-01.abc

Only happens on win and win64.
Flags: in-testsuite+
Flags: flashplayer-triage+
Flags: flashplayer-qrb?
Whiteboard: skipped-in-testconfig
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Flags: flashplayer-qrb? → flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.1
Attachment #432018 - Attachment mime type: application/octet-stream → text/plain
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.
Another bug touching on free list management is bug #523582.
blocks are supposed to be eagerly coalesced, that's the root problem
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.
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)
Whiteboard: skipped-in-testconfig → Has patch, skipped-in-testconfig
Attachment #432962 - Flags: review?(treilly) → review+
tamarin-redux-argo changeset:   3838:dd364ce52291
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 554890
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.
test removed from testconfig, pushed to argo changeset 3915	7e5993aa597a
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.