554890 - assertion in MMgc running a performance testcase

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Brent Baker]

The assertion added to GCHeap.cpp:906 is causing an intermittent failure running the following testcases:

test/performance/language/string/static_latin1_array_50.abc
test/performance/language/string/static_latin1_array_100.abc

This seems to be more reproducible on the mac 10.4 performance machine but I have reproduced once on a mac 10.5 machine

asteammac2:~/temp build$ ./avmshell_d static_latin1_array_50.abc 
Assertion failed: "(((next - blocks < (intptr_t)blocksLen)))" ("/Users/build/buildbot/tamarin-redux/mac-intel-10_5/repo/MMgc/GCHeap.cpp":906)

Comment 1

[Brent Baker]

QRB: This is an injection and should be considered as blocking the merge this week.

Comment 2

[Brent Baker]

If the testcase is run with a release shell, you will intermittently receive a "Bus Error"

Comment 3

[Brent Baker]

64 runs of static_latin1_array_50.abc produced 0 failures with tr-argo rev 3837 and 11 failures with build 3838 (which is when the assertion was added to the codebase).

http://asteam.macromedia.com/hg/tamarin-redux-argo/rev/3838

Comment 4

[Lars T Hansen]

So far unable to repro on 10.6 (64 runs of case 1), will keep trying.

10.4 has different allocation patterns because it does not use virtual memory (due to a limitation in the OS from what I understand, Tommy knows more).  Allocation patterns, including computer speed, could impact reproducibility of this bug.

Comment 5

[Brent Baker]

I ran about 150 times on my local 10.5 mac and was not able to repo, repos no problem on 10.4

Comment 6

[Lars T Hansen]

Well, dang it :-)  I have a 10.4 machine (PPC) but it's on the other side of town.  I've looked at the assert and I think it's right.  Will keep investigating.

Comment 7

[Tommy Reilly]

crash repros for me on 10.4, goes away with rev 3837, can't get it to crash in a debug build or release in gdb, grr...

Comment 8

[Lars T Hansen]

Attachment: Sentinels should always be uncommitted

Recent assertions count on sentinels being uncommitted.  I believe that is a useful invariant, as we count on eagerly coalescing committed blocks; there's no reason to confuse the issue.

(The alternative is to allow sentinels to be committed or uncommitted but then check whether we're looking at a sentinel in cases where we're examining whether a block is committed.  That could work too; I'm not opposed.  Let me know.)

This patch may or may not fix the present bug.  It may or may not fix a related assertion that Ed ran into (no bugzilla for that one yet, trying to repro.)  It is offered only because it seems like a reasonable change.

Comment 9

[Tommy Reilly]

crash seems to have something to do with Decommit, can't repro with returnMemory turned off

Comment 10

[Lars T Hansen]

(In reply to comment #9)
> crash seems to have something to do with Decommit, can't repro with
> returnMemory turned off

But we're not using Decommit on 10.4 are we?  We're running w/o virtual memory on that platform.

Comment 11

[Tommy Reilly]

Decommit removes completely free regions when running w/o virtual memory, ReturnMemoryToOS would be a better name

Comment 12

[Tommy Reilly]

Attachment: fixes crash

Comment 13

[Tommy Reilly]

tamarin-redux-argo: changeset:   3871:ba8dc8f024
tamarin-redux: changeset:   4174:ba8dc8f024cb

Submitting pre-review b/c its trivial and to unblock integration efforts of east coast early risers.

Comment 14

[Lars T Hansen]

I'm probably not going to push the sentinel fix, it does not seem to be required at this time.  Close at will.

Comment 15

[Brent Baker]

Verified in tr-argo 3874