480641 - Creating porting APIs for MMgc

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.6) Gecko/2009011912 Firefox/3.0.6
Build Identifier: 

MMgc depends on platform functionality for managing memory.  The platform-specific code needs to be abstracted to VMPI APIs to make porting easier.

Reproducible: Always

Comment 1

[Rishit Shah]

Attachment: patch [v1] - mmgc porting apis

Summary of changes -

- Added APIs for malloc/free, mmap functionalities, aligned malloc/free, lock/unlock capabilities

- Got rid of GCHeap[Win/Mac/Unix].cpp.

- Added new directory "platform" under MMgc

- New files under "platform" MMgcPort[Win/Mac/Unix].cpp contain VMPI implementations for their respective platforms

- GCSpinLock[Win/Mac/Unix].h go away.  The corresponding VMPI implementations live in SpinLock[Win/Mac/Unix].cpp

- GCSpinLock class is removed since that is replaced by vmpi_spin_lock_t - an opaque data type for platform-specific lock

Some considerations:
- Is vmpi_spin_lock_t name okay?
- Added verbose comments for VMPI_lockAcquire.  Not sure if we need to be that detailed.
- Wanted to keep lock VMPI implementation separate from other ones.  Hence a separate file.
- Should we have platform-specific folders under platform [or root MMgc] for the porting implementations?

Comment 2

[Tommy Reilly]

Comment on attachment 365481 [details] [diff] [review]
patch [v1] - mmgc porting apis

GCHEAP_LOCK should go away and use MMGC_LOCKING instead, I wanted to get rid of all non-prefixed CPP symbols

Comment 3

[Lars T Hansen]

I find this to be unreviewable, it appears to assume the existence of files that are not here, and the absence of some files that aren't here.

Comment 4

[Lars T Hansen]

Comment on attachment 365481 [details] [diff] [review]
patch [v1] - mmgc porting apis

tentatively approved.  awaiting new patches, i will take care of landing it.

Comment 5

[Rishit Shah]

Attachment: Patch [v2] - updates from code review

Incorporating patch v1 feedback.

Had to derive SpinLock[Platform] classes from GCAllocObject.  If this is not acceptable then the alternative is to implement private new/delete for those classes.

Had to fix Win64 project.

Comment 6

[Tommy Reilly]

Comment on attachment 367300 [details] [diff] [review]
Patch [v2] - updates from code review

Can we remove the GCHeap virtual memory functions and just call the VMPI functions directly?  Otherwise looks great!

Comment 7

[Rishit Shah]

Attachment: patch [v3] - based on Tommy's feedback from review pf [v2]

Comment 8

[Rishit Shah]

I have made the changes based on Tommy's comment.  If this confirmed then I'll a volunteer to push this patch.

The patch is created fresh based on March 16 tamarin-redux.

I can re-create the patch from latest changes if required.

Comment 9

[Lars T Hansen]

Comment on attachment 367908 [details] [diff] [review]
patch [v3] - based on Tommy's feedback from review pf [v2]

Approved, but I'm not happy with some aspects of the design.  In particular, breaking individual virtual memory functions into separate VMPI functions, rather than trying to create a virtual memory abstraction that can hold all of these, seems like the wrong choice to me.  Also, typedeffing the lock type as void* rather than defining a distinguished abstract base class that locks can inherit from introduces more type-unsafety than what is probably reasonable, even if it does provide for some efficiency in the representation of locks.

I'm approving because the patch does not make anything worse and I think more MMgc cleanup will have to be undertaken in any case, but I don't want future work to hold up this patch.

Comment 10

[Tommy Reilly]

fixed in 1637