566843 - GCHeap free list handling appears imprecise

Status: RESOLVED WONTFIX

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

Description

[Lars T Hansen]

Attachment: Preliminary patch

It's not clear that the number of free lists computed is correct (relative to the comments in the file); it should be one more.  This is compensated for in GetFreeListIndex apparently, but it need not be so.  Also, there are further invariants on the free lists that should definitely be asserted in CheckFreeLists.

Attaching a patch with some preliminary work on tamarin-redux.

Comment 1

[Ruchi Lohani]

Total freelists are coming to a count of 31. So the indexes vary from 0..... 15, 16........ 29, 30

The comment in the code above the kFreeListCompression definition is not holding correct as the first bin (freelist with index 16) will hold for 7 sizes (17..... 23). 

Also, the assert in GetFreeListIndex() : GCAssert(v > 15 && v < kHugeThreshold); doesn't look correct to me. So either this is not right or my understanding is cheating on me. 

It should be : GCAssert(v > (kUniqueThreashold-1) && v < kNumFreeLists); ?

Comment 2

[Ruchi Lohani]

Forgot to mention: the assert in the above comment is added in the preliminary patch. Its not currently in the code.

Comment 3

[Ruchi Lohani]

There is one more inconsistency I see with the new assert conditions added to the CheckFreeLists. Since the condition that each bin from list index 16 will be the bin for 8 distinct sizes is not true for the first bin, the minsize calculated in the conditions below will not be correct.
                minsize = kUniqueThreshold + (i - kUniqueThreshold)*kFreeListCompression;
1363
                maxsize = kUniqueThreshold + (i - kUniqueThreshold)*kFreeListCompression + (kFreeListCompression - 1);


One possible things would be to let every intermediate bin(freelist) get 8 distinct sizes and consider the last free list for (size > kHugeThreshold) rather than (size >= kHugeThreshold)?

Comment 4

[Ruchi Lohani]

Attachment: Patch with further modifications

I am not asking review for this patch rightnow. This is more to put all in code what I had talked in the bug notes. Further discussion might lead to more modifications and a cleaner patch probably.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.