Remove literal constants from MMgc

RESOLVED FIXED in Q3 11 - Serrano

Status

P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: treilly, Assigned: lhansen)

Tracking

unspecified
Q3 11 - Serrano
Bug Flags:
flashplayer-qrb +
flashplayer-bug -

Details

(Whiteboard: has-patch)

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
Just noticed a 0xffff that should be a 0xfff in MarkItem time to stop the madness!
(Assignee)

Comment 1

9 years ago
Would be good to vet this for Argo, so targeting, even though the bug here is minor (at most we get excess retention).
Priority: -- → P2
Target Milestone: --- → flash10.1
(Assignee)

Updated

9 years ago
Assignee: nobody → treilly

Updated

9 years ago
Status: NEW → ASSIGNED
Flags: flashplayer-qrb+

Comment 2

9 years ago
Deferring to 10.2.
Target Milestone: flash10.1 → flash10.2
(Assignee)

Comment 3

9 years ago
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.
(Assignee)

Comment 4

9 years ago
Found another 0xffff bug, poaching.
Assignee: treilly → lhansen
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 5

9 years ago
Created attachment 444959 [details] [diff] [review]
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.
Attachment #444959 - Flags: review?(fklockii)
(Assignee)

Comment 6

9 years ago
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.
(Assignee)

Updated

9 years ago
Whiteboard: has-patch
(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.
(Assignee)

Comment 8

9 years ago
(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 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.)
Attachment #444959 - Flags: review?(fklockii) → review+
(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!
(Assignee)

Comment 11

9 years ago
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.
(Assignee)

Comment 12

9 years ago
Created attachment 449000 [details] [diff] [review]
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.
Attachment #449000 - Flags: review?(fklockii)
Comment on attachment 449000 [details] [diff] [review]
Assert that the block size is 4K at strategic spots

r+

nit: "occurences" has two r's.
Attachment #449000 - Flags: review?(fklockii) → review+
(Assignee)

Comment 14

9 years ago
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
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

8 years ago
Flags: flashplayer-bug-
You need to log in before you can comment on or make changes to this bug.