485961 - MMgc code cleanup items based on MMgc code review

Status: VERIFIED FIXED

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

Description

[Rishit Shah]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7) Gecko/2009021906 Firefox/3.0.7
Build Identifier: 

Following is a list of minor cleanup tasks within MMgc based on Lars' review of the code -

#1 - MMgc::DebugSize() should be inlined.

#2 - Modify FixedAlloc::CreateChunk's signature to return void since the function never returns NULL.  (OOM will cause the code to throw an exception)

#3 - FixedMalloc::Alloc and GCHeap::Alloc never return NULL.  So remove NULL checks for the return values from these functions.

#4 - Empty source files (MMgc.cpp, mmgc_stdint.h, GCGlobalNew.cpp, avmplus_stdint.h) should be deleted.

#5 - Replace naked int and unsigned int usage in MMgc with int32_t and uint32_t respectively.

#6 - Remove redundant code from GC::Sweep.


Reproducible: Always

Comment 1

[Rishit Shah]

DebugSize() is a function call only for MEMORY_INFO builds.  Is inlining it required then?

Also, for MMgc DLL case inline would not be applicable anyways.

Comment 2

[Lars T Hansen]

Re #3, my fault, but GCHeap::Alloc can return NULL if its expand parameter is false, it is involved in a complicated dance with the collection policy (it forces a collection to completion if a block can't be allocated).  There are other places in the code - I forget where, I'll try to find them - that assume that it won't return NULL, but that's only valid if the expand parameter is true, and even in that case it's necessary to check that GCHeap::Alloc does not violate that requirement.  So I would suggest you keep this work item to  FixedMalloc::Alloc only.

Comment 3

[Lars T Hansen]

(In reply to comment #1)
> DebugSize() is a function call only for MEMORY_INFO builds.  Is inlining it
> required then?

IMO it's desirable for debug builds to be efficient & small, even if they will be slower and larger.  You're right, it's not a big deal, but debugging needs to be efficient.

> Also, for MMgc DLL case inline would not be applicable anyways.

Good point, if it's an API function.  That reminds me to ask a question about whether there is a use case for MMgc DLL builds any more.  My gut feeling is "no".

Comment 4

[Rishit Shah]

Need to cleanup uses of MMGC_PORTING_API now that we have VMPI infrastructure.

Comment 5

[Rishit Shah]

Re (In reply to comment #2)
> Re #3, my fault, but GCHeap::Alloc can return NULL if its expand parameter is
> false, it is involved in a complicated dance with the collection policy (it
> forces a collection to completion if a block can't be allocated).  

I took care in making changes only where expand parameter would be true.

Comment 6

[Rishit Shah]

Attachment: [v1] patch

contains changes for items 1-6 listed under description.

Comment 7

[Tommy Reilly]

Comment on v1 patch

This patch highlight a bunch of dead code.  The new profiler doesn't store anything in the allocation anymore so these comments are no longer true:

 				// 2nd 4 bytes - alloc stack trace
 				// 3rd 4 bytes - free stack trace

The code related to writing to a delete object is wrong, please see the DownwardlyMobile branch for how it should look.

What's with sweepResults, I thought that moved into Lars policy manager?

The "Deleted item write violation!" violation code is duplicated thrice, can we generalize and implement once?

The code in GCWeakRef is wrong, that should be using GetStackTrace and storing a StackTrace* and be ifdef'd MMGC_MEMORY_INFO not DEBUG

otherwise looks good!

Comment 8

[Tommy Reilly]

Comment on attachment 371302 [details] [diff] [review]
[v1] patch

see comments on bug

Comment 9

[Rishit Shah]

(In reply to comment #7)

> This patch highlight a bunch of dead code.  The new profiler doesn't store
> anything in the allocation anymore so these comments are no longer true:
> 
>                  // 2nd 4 bytes - alloc stack trace
>                  // 3rd 4 bytes - free stack trace

Fixed it.
 
> The code related to writing to a delete object is wrong, please see the
> DownwardlyMobile branch for how it should look.

Done.


> What's with sweepResults, I thought that moved into Lars policy manager?

That change is from your response to the parent bug -
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  -------  Comment #38 From  Tommy Reilly   2009-03-20 03:54:41 PDT   (-) [reply] -------

Indeed I dont recall writing code to interate over those list twice, especially
when we whack the list after the first iteration.  I think the sweepResults++
should be in the first iteration.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I am not aware of policy manager.


> The "Deleted item write violation!" violation code is duplicated thrice, can we
> generalize and implement once?

Done. Though for GCAlloc's there is a hit for extra cycles of iteration because there could be 2 sets of poison bytes.

 
> The code in GCWeakRef is wrong, that should be using GetStackTrace and storing
> a StackTrace* and be ifdef'd MMGC_MEMORY_INFO not DEBUG

Don't see obj_creation being used anywhere right now.  Ignoring that, StackTrace is defined only for MMGC_MEMORY_PROFILER.  So the suggested change doesn't work for MMGC_MEMORY_INFO w/o MMGC_MEMORY_PROFILER.  The usage of obj_creation would help understand its utility and hence the required change.

Comment 10

[Lars T Hansen]

Policy manager appeared in GC.h / GC.cpp over the last couple of weeks, it centralizes a lot of accounting and makes policy decisions.  Not all accounting is performed there yet, as I've been reluctant to assimilate accounting that is only used for logging, not for policy, but there's no particular reason we couldn't centralize everything - fewer bugs would likely result, as concerns are separated better.

Comment 11

[Tommy Reilly]

Its useful when debugging weak refs to know the allocation stack of the referent b/c later when you want it the referent may have gone away and the GCWeakRef just contains NULL.   basically from a debugger you would go invoke MMgc::PrintStackTrace(wr->obj_creation)

It appears I may be confused about sweepResults, if it didn't move anywhere and you are just fixing existing code ignore my comment

Comment 12

[Rishit Shah]

Attachment: [v2] patch

Changes based on feedback from review of v1 patch.

New changes in this patch include -
1. Fixing up deleted write code and removing redundancy
2. Rectifying outdated comments on DebugDecorated layout of allocated memory
3. Fix obj_creation in GCWeakRef to enable retrieval of allocation stack trace in the debugger.

Comment 13

[Rishit Shah]

changeset 1733	8cf8220c0be6

Comment 14

[Tamarin Bot]

changeset: 7210:6cb055da822a
user:      Felix S Klock II <[e-mail]>
summary:   Bug 485961: fix old typo in comment (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/6cb055da822a