Just noticed a 0xffff that should be a 0xfff in MarkItem time to stop the madness!
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
Deferring to 10.2.
Target Milestone: flash10.1 → flash10.2
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.
Found another 0xffff bug, poaching.
Assignee: treilly → lhansen
OS: Mac OS X → All
Hardware: x86 → All
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)
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.
(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.
(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!
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.
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+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.