551840 - mmgc intermittent assert running spidermonkey performance test

Status: VERIFIED FIXED

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

Description

[Chris Peyer]

Attachment: 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.

Comment 1

[Chris Peyer]

Attachment: abc that causes intermittent assert

Comment 2

[Chris Peyer]

Attachment: Testcase from acceptance suite.

Comment 3

[Lars T Hansen]

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

[Lars T Hansen]

Another bug touching on free list management is bug #523582.

Comment 5

[Tommy Reilly]

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

Comment 6

[Tommy Reilly]

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

[Lars T Hansen]

Attachment: 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.

Comment 8

[Lars T Hansen]

tamarin-redux-argo changeset:   3838:dd364ce52291

Comment 9

[Brent Baker]

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

[Chris Peyer]

test removed from testconfig, pushed to argo changeset 3915	7e5993aa597a