Work items I know about: - factor inline functions into a new file, GCHeap-inlines.h - document all methods and fields - retain as 'public' only methods and properties that are accessed from outside MMgc; all other methods and properties should be private, and GCHeap should friend other mmgc classes as necessary - it is possible that the definitions of Region and HeapBlock can be moved into GCHeap.cpp as these are only used in that one file; doing so may help make GCHeap.h easier to understand There might be other items; discuss. It would be good to get this done sooner rather than later, hence the P2.

Cut-and-pasting comments I put in my notes when I was looking over this header file some days ago. They don't all represent real tasks, and some of them are probably more a commentary on *me* rather than the code base. :) GCHeap.h -------- * GCManager::tryAddGC does not document meaning of its return value; nor does BasicList::TryAdd. * GCManager::gcs exposes collectors; okay? * Comment above class GCHeap says "Only Windows is currently supported, ..."; - surely that is lie? Casts shadow on comments... - (that note dates from rev 1) * Shouldn't kNumFreeLists computation use a division that rounds *up* after dividing by kFreeListCompression? - I suppose if no one observes kNumFreeLists (or kFreeListCompression always evenly divides the threshold delta) then we're safe. * GCHeap::SignalCodeMemoryAllocation: seems like it may be hard to connect to associated alloc invocation when multi-threaded. * GCHeap::FreeNoProfile is missing doc * Interesting contortions in GCHeap::CheckForAllocSizeOverflow, and its all with 64-bit arithmetic even on 32-bit platforms... (potential opportunity for teensy perf gain on such checks, but unlikely to matter) - likewise for CheckForCallocSizeOverflow, though that's less obvious how to fix (switch from 64-bit mult to 32-bit div, yuck?) - heh "yay" disjunct-spacing in CheckForCallocSizeOverflow * GCHeap::log_percentage is missing doc * GCHeap::IsAddressInHeap is missing doc; should at least say if cheap or not * GCHeap::QueryCanReturnToNormal refers to a "soft limit"; needs defn * GCHeap::ExpandHeap again may be hard to work with once multi-threaded * (private method commentary below) * AllocBlock/AllocCommittedBlock/CreateCommitedBlock, which is which? - not such a big deal as they are all private, so UseTheSourceLuke * likewise for PruneDecommittedBlock/FreeBlock/FreeInternal/RemoveBlock * wait, numHeapBlocksToNumBlocks(..), two are not synonymous? * comment above GCHeap::NewRegion is unintelligible. - (its a recent comment, rev 2745 from bugz 488738)

(In reply to comment #0) > - it is possible that the definitions of Region and HeapBlock can be moved into > GCHeap.cpp as these are only used in that one file Cannot really do it for Region until its use elsewhere (see bug 542813) is addressed. I'm having a bear of a time getting my Windows CE dev environment set up, so I might be punting on that particular refactoring for a while.

Also HeapBlock cannot be trivially moved because of the member definition "HeapBlock freelists[kNumFreeLists];" in GCHeap. (Unless you want me to move the freelists array somewhere else?)

I think a light touch is OK for now, we can address those issues later. Providing a proper callback API for WinCE to use would be good, but we're probably going to redo the page descriptor table anyway, so no sense in worrying too much now.

I have a separate patch for the player's XCode project. (its a pretty trivial thing to add the new file though.)

This is a pretty simple set of access restrictions. The member functions' accessibility has been left unchanged for now; this is essentially restricting a number of named constants. (More interesting are the constants whose access could not yet be restricted.)

arguably this should have been part of attachment 445187 [details] [diff] [review]. And arguably I should go through this process for Eclipse. (But not Visual Studio, as TR does not have a MS Visual Studio "Solution" (bleah) file, right?

Comment on attachment 445189 [details] [diff] [review] restricts access to some members in GCHeap.h hey Lars: one issue is that I left kOSAllocThreshold public because the selftest ST_mmgc_gcheap.cpp is referring to it. But one alternative would be to friend the selftest. Is there a third alternative? Which option is best?

Comment on attachment 445187 [details] [diff] [review] moves GCHeap.h inline functions into their own file (decided to go back and move the rest of the inline functions over; not sure why i skipped them before. If I change my mind back, I'll commit my reasoning to a comment rather than get stuck in a loop.)

Comment on attachment 445197 [details] [diff] [review] xcode project changes for GCHeap-inlines.h until GCHeap-inlines.h exists, there's no need to review this.

More aggressive refactoring; moves all REALLY_INLINE, inline, and implicitly inline methods into a new GCHeap-inllines.h file.

Attempt to restrict access as best I could manage without changing client source code. I did some rearrangement of method declarations to try to group related functionality together so that the grouping comments I used could make some sense. I also put occasional notes about what client code I discovered was relying on some member being accessible, to make it a little easier to track down those uses if we want to try to restrict access further in the future. (This is questionable though, especially keeping it as a comment in the header file rather than documenting it elsewhere, or just forcing the future worker track down the references him/herself rather than being potentially misled by outdated comments here.) Tried to adopt consistent style of marking areas with either private: /* -- Area Title -- */ or public: /* -- Area Title -- */ so that each area has a "default access" associated with it, and it is the responsibility of access-mode-pokers to restore the default mode at the end of the hole they introduce. Has been built against several player configurations, but not all. (I'm still working on figuring out our Build Forge service).

(In reply to comment #1) > * Comment above class GCHeap says "Only Windows is currently supported, ..."; > - surely that is lie? Casts shadow on comments... > - (that note dates from rev 1) Comments need serious cleanup in this file. > * Shouldn't kNumFreeLists computation use a division that rounds *up* after > dividing by kFreeListCompression? > - I suppose if no one observes kNumFreeLists (or kFreeListCompression always > evenly divides the threshold delta) then we're safe. Yes, good eye. There are several dodgy comments and computations here, I have some notes elsewhere, probably warrants a followup bug. I will create one. > * GCHeap::SignalCodeMemoryAllocation: seems like it may be hard to connect to > associated alloc invocation when multi-threaded. Can you elaborate? > * Interesting contortions in GCHeap::CheckForAllocSizeOverflow, and its all > with 64-bit arithmetic even on 32-bit platforms... (potential opportunity for > teensy perf gain on such checks, but unlikely to matter) If substandard(?) mobile compiler generates a function call for 64-bit arithmetic rather than inline code then it may make sense to optimize.

(In reply to comment #7) > > And arguably I should go through this process for Eclipse. (But not Visual > Studio, as TR does not have a MS Visual Studio "Solution" (bleah) file, right? platform/win32/avmplus2008.sln.

(In reply to comment #8) > (From update of attachment 445189 [details] [diff] [review]) > hey Lars: one issue is that I left kOSAllocThreshold public because the > selftest ST_mmgc_gcheap.cpp is referring to it. But one alternative would be > to friend the selftest. Is there a third alternative? Which option is best? When this has come up in the past I've been in favor of friending the selftest, which is far from ideal but OK as long as the number of selftests that need access is small.

Comment on attachment 446025 [details] [diff] [review] moves GCHeap.h inline functions into their own file Again, I'm fairly sure the /*REALLY_INLINE*/ annotation does not add anything substantial in almost all cases, and could go. Maybe poll a couple other people. (Also I prefer it on the line above when it's there so that it doesn't clutter up the declaration, as I do with /*static*/ in cpp files, but there's no accounting for taste.)

(In reply to comment #16) > (From update of attachment 446025 [details] [diff] [review]) > Again, I'm fairly sure the /*REALLY_INLINE*/ annotation does not add anything > substantial in almost all cases, and could go. Maybe poll a couple other > people. Having the annotation points a reader to look in the *-inlines file for the associated definition. I assume you know this and are just saying that it is not worth the expense of putting the annotation in front of the reader . . . > (Also I prefer it on the line above when it's there so that it doesn't clutter > up the declaration, as I do with /*static*/ in cpp files, but there's no > accounting for taste.) Hmm. I suppose that if it were moved to its own line, I'd tend to be less inclined to keep all/any of them. (I'm willing to pay the cost of reading extra tokens as I skim a header, but a line with just the annotation is a more serious expense in screen real estate, since that's fewer lines I can fit in a buffer to view at once. . . We'll see what others think, I guess. . .

(In reply to comment #13) > > * GCHeap::SignalCodeMemoryAllocation: seems like it may be hard to connect to > > associated alloc invocation when multi-threaded. > > Can you elaborate? I did not understand what the Signaling was for at the time when I read the header file and wrote that comment. Now I understand that it is just incrementing some memory usage counters; that does not present any threading issues.

(In reply to comment #17) > (In reply to comment #16) > > (From update of attachment 446025 [details] [diff] [review] [details]) > > Again, I'm fairly sure the /*REALLY_INLINE*/ annotation does not add anything > > substantial in almost all cases, and could go. Maybe poll a couple other > > people. > > Having the annotation points a reader to look in the *-inlines file for the > associated definition. I assume you know this and are just saying that it is > not worth the expense of putting the annotation in front of the reader . . . I rely (99% of the time, at least) on my IDE to find the function for me. (The *-inlines files aren't a service to the reader primarily, they provide the ability to have circular references among classes defined in separate files, and for those classes' inline functions to reference the other classes, without upsetting the compiler, ie, it's a workaround for a language oddity.)

rebased, little more cleanup, and added GCHeap-inlines.h to VS 2008 vcproj.

Rebased and cleaned up. Lets land this on its own, and soon. (Decided Lars was right that /*REALLY_INLINE*/ annotations do more harm than good, and confirmed via straw poll in chat-room)

Comment on attachment 456160 [details] [diff] [review] moves GCHeap.h inline functions into their own file Ship it!

(In reply to comment #22) > (From update of attachment 456160 [details] [diff] [review]) > Ship it! (push please)

(In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 456160 [details] [diff] [review] [details]) > > Ship it! > > (push please) tamarin-redux changeset: 4910:7c26b8022ce7

This patch is a mix of work I did back in April and from the past few days, so the narrative tone may be a little uneven. I don't know a good way to validate this work, beyond eyeballs, and its really too much to be reviewed all at once. Let me know if you would prefer that I split it into smaller chunks (e.g. one decomposition would be to do the public API first, then the private one.) Also, it seems like tools like Eclipse encourage putting documentation above the implementation of a method, rather than with its declaration in the header (because they pop-up the implementation when you do mouse-over, and will include comments above the implementation when present). Is there a way to make Eclipse present header docs? Or, alternatively, would we prefer to put the documentation for the private API with the implementations instead of in the header?

(In reply to comment #25) > Created attachment 479534 [details] [diff] [review] > add documentation for nearly all methods > > This patch is a mix of work I did back in April Obviously bogus (off by one error) from ticket history.

I did not want these two notes lost to the four winds, but also did not think they were severe enough to get their own ticket (at least, not yet). Tom: I'm pretty sure these are both bugs. Any thoughts on severity?

(In reply to comment #25) > Also, it seems like tools like Eclipse encourage putting documentation above > the implementation of a method, rather than with its declaration in the header > (because they pop-up the implementation when you do mouse-over, and will > include comments above the implementation when present). Is there a way to > make Eclipse present header docs? I don't know. I don't know how many of use use Eclipse, or why. > Or, alternatively, would we prefer to put > the documentation for the private API with the implementations instead of in > the header? API documentation belongs in the header. If there are implementation notes they belong with the implementation.

Comment on attachment 479534 [details] [diff] [review] add documentation for nearly all methods I'm on the fence but will SR- because there appears to be missing work even for the comments that were added by this patch. There are also many cleanup items but I'd be content with creating followup entries for those. Followup item (required): Capitalize and punctuate and generally clean up existing comments, see eg comment on heapLimit in GCHeap.h. Your addition to the comment on eagerSweeping is wrong, I think - eager sweeping uses the lists of sweepable blocks just like lazy sweeping, but with eager sweeping they are all swept at the end of the GC, not as blocks are needed for allocation. Your addition to the comment on gcLoad is not correct, the series of load factors does not need to be decreasing (at least that was not the intent!) Spelling error ("gloal") on EnterLockDestroy comment. Clean up blank lines after AllowDestruct? In comment for AddOOMCallback you added a @see (good!) but elsewhere you're consistently using '()' following the function name, not so here. The TODO above kNativePageSize should be folded into the block comment I think, cleaner that way. Missing documentation for GetStackEntryAddress. If GetEnterFrame is only referenced by the sampler then it should be #ifdef SAMPLER. Otherwise the parenthetical comment should be removed. The comments for the stack entry functions are inadequate by themselves; if good documentation exists elsewhere then a cross reference is needed here, otherwise a required followup item is to create that documentation and cross reference it as appropriate. Style for comment: CheckForOOMAbortAllocation and QueryCanReturnToNormal use un-asterisked comments (missing asterisk column). Ditto others. Documentation for DestroyInstance is inadequate: we don't care who calls it, but what the preconditions, postconditions, and (if not implied by the other two) effects are. Historical note on ExpandHeapInternal not useful, and comment probably indadequate overall. The question marks in the comments for isSentiel need to turn into a FIXME + bugzilla bug to investigate; if we're unable to document the code we need to figure out what it does. The 'zero' parameter on AllocHelper, AllocBlock, AllocCommittedBlock must be documented better: when must the caller pay attention to a returned value, for example? What's the meaning of the alignment argument to CreateCommittedBlock given that the return value is specified as 'not necessarily aligned'? Convert comments for RemoveBlock, LargeAlloc, etc. The comment for Commit is a little scant. It hints at a larger design: a lifecycle through which blocks move. That design should be documented if it is not; once it's documented it should be cross-referenced from here. If ValidateHeapBlocks is DEBUG-only it should be #ifdef'd. Most comments missing in data section.

Comment on attachment 479545 [details] [diff] [review] some todo's in the cpp itself. The first one is an obvious bug, just fix it, its possibly never been hit because most VM's don't actually allocate physical memory on commit they just twiddle some bits and let page faults do the real work. +1 on the assert. The only time we call EnsureFreeRegion is when are decommiting so the idea is we should be able to allocate a block just fine.

Comment on attachment 479534 [details] [diff] [review] add documentation for nearly all methods (taking tommy off; his review can wait until after I address Lars' feedback.)

(In reply to comment #28) > I don't know. I don't know how many of use use Eclipse, or why. AFAIK, very few people are using Eclipse on a regular basis (for Tamarin) -- it's not the sweet spot to optimize for.

(In reply to comment #29) > Most comments missing in data section. As part of fixing this, definitely need to explain how the Region array and HeapBlock array collaborate to maintain a sparse representation; see Bug 445780 for some of the discussion around this.

(releasing back to the central bug pool.)

Tamarin is a dead project now. Mass WONTFIX.

Tamarin isn't maintained anymore. WONTFIX remaining bugs.