544298 - Remove literal constants from MMgc

Status: RESOLVED FIXED

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

Description

[Tommy Reilly]

Just noticed a 0xffff that should be a 0xfff in MarkItem time to stop the madness!

Comment 1

[Lars T Hansen]

Would be good to vet this for Argo, so targeting, even though the bug here is minor (at most we get excess retention).

Comment 2

[Dan Smith]

Deferring to 10.2.

Comment 3

[Lars T Hansen]

Constants of note are: 4096, 4095, 0xFFF, 0x1000, at least.  Wouldn't hurt to look for 0xFFFF, 0x10000, 8192, 8191, 0x2000, 0x1FFF as well.

Extra points for getting rid of the redundant definitions of kBlockSize - last I looked there was one in GCHeap, another in GC.  Only the former should be there.

Comment 4

[Lars T Hansen]

Found another 0xffff bug, poaching.

Comment 5

[Lars T Hansen]

Attachment: Clean up literal uses of kBlockSize

(Tommy, please review if you have the cycles but don't feel obligated.)

This gets rid of the immediate uses of values that correspond to GCHeap::kBlockSize and GCHeap::kBlockSize - 1.  I have corrected two instances where I'm certain 0xFFFF should have been 0xFFF.

There are further issues; see following comments.

Comment 6

[Lars T Hansen]

Follow-on items are at least these:

FixedAlloc, GCAlloc, GCMarkStack, and ZCT all "know" the block size and encodes it in constants that are derived in more interesting ways than blocksize-1: for example, FixedAlloc and GCAlloc know how many objects of a certain size class will fit on a page and encodes this in tables; the zct knows how big a page is and uses this for its data structures; the mark stack the same thing.

In the GC, a lot of code knows that four bits are used for the per-object mark bits, and this fact is spread around a fair bit.  It's not hard to abstract it - I've done it for some experimental work - and it would be worth looking into doing so.  Also shows up as 0x33333333 in a few places.

In the GC, some code (SetPageMapValue, GetPageMapValue, ClearPageMapValue, among others) has some interesting constants that probably relate to block sizes and the fact that two bits are used per page for the GC's page tables.  More study is needed.

MarkGCPages also has some interesting constants.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #5)
> Created an attachment (id=444959) [details]
> Clean up literal uses of kBlockSize

Its a lot to look over.

An early comment: do we have a divideRoundingUp(..) template function or macro to encapsulate the pattern of ((X+Y-1)/Y) ?   Its not strictly necessary for this patch, its just part of trying to clean up the code further.

Comment 8

[Lars T Hansen]

(In reply to comment #7)
> An early comment: do we have a divideRoundingUp(..) template function or macro
> to encapsulate the pattern of ((X+Y-1)/Y) ?   Its not strictly necessary for
> this patch, its just part of trying to clean up the code further.

None to my knowledge.

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 444959 [details] [diff] [review]
Clean up literal uses of kBlockSize

r+.  Lars confirmed that the two 0xFFFF replacements (as opposed to 0xFFF) were bugs.

We may want a divideRoundingUp template function at some point.  (Perhaps once someone suggests a better name to assign to it.)

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #9)
> (From update of attachment 444959 [details] [diff] [review])
> r+.  Lars confirmed that the two 0xFFFF replacements (as opposed to 0xFFF) were
> bugs.

Misspoke; the instances of 0xFFFF were bugs, and their replacements were bug fixes!

Comment 11

[Lars T Hansen]

tamarin-redux changeset:   4755:b16a486d753a

I will keep this open for now; I want to create another patch that inserts GCAsserts in the cases where I think values are derived from a known block size, as noted earlier.

Felix, I'll let you worry about whether you want to introduce utility functions for rounding and so on, I support the idea.

Comment 12

[Lars T Hansen]

Attachment: Assert that the block size is 4K at strategic spots

I'm not in the mood to fix the problem right now, so just assert that code that uses constants derived in various complicated will only be run when the block size is in fact 4K.  I doubt we'll change that anyway.

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 449000 [details] [diff] [review]
Assert that the block size is 4K at strategic spots

r+

nit: "occurences" has two r's.

Comment 14

[Lars T Hansen]

Comment on attachment 449000 [details] [diff] [review]
Assert that the block size is 4K at strategic spots

Fixed two instances of 'occurences'.

tamarin-redux changeset:   4763:b32af36afafb