13.55 KB, application/octet-stream
14.27 KB, application/octet-stream
2.86 KB, text/plain
7.40 KB, patch
|Details | Diff | Splinter Review|
Created attachment 432016 [details] 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.
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.
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)
Whiteboard: skipped-in-testconfig → Has patch, skipped-in-testconfig
tamarin-redux-argo changeset: 3838:dd364ce52291
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.