503134 - VMPI locking api using heap allocations is problematic and sub-optimal

Status: VERIFIED FIXED

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

Description

[Tommy Reilly]

We should go back to having clients declare the space for the lock (ie a vmpi_spin_lock_t) and passing the address of it to the lock functions.     Ie:

vmpi_spin_lock_t* VMPI_lockCreate -> void VMPI_lockCreate(vmpi_spin_lock_t *l);
void VMPI_lockDestroy(vmpi_spin_lock_t *l);
void VMPI_lockAcquire(vmpi_spin_lock_t *l);
void VMPI_lockRelease(vmpi_spin_lock_t *l);

Also propose to move win32 specific locking diagnostics into platform agnostic code to all platforms benefit.

Comment 1

[Lars T Hansen]

What is problematic and sub-optimal about it?

Comment 2

[Tommy Reilly]

a) it introduces heap allocations where previously we had none
b) the allocator itself uses these locks making shutdown complicated (right now we free all memory then call lockDestroy in GCHeap which I can't believe isn't crashing)
c) in the player there are cases where a lock is acquired and they delete the heap while they have the lock (to prevent two thread from trying to delete the heap), this is crashing

In general tying a low level construct like a lock to heap allocation instead of giving flexibilty to the client on where the lock lives seems unnecessarily limiting and is contrary to all existing OS locking APIs that I know of.

Comment 3

[Tommy Reilly]

Attachment: First cut, still needs sandbox testing. Fixed SpinLockSymbian line endings.

Comment 4

[Lars T Hansen]

Comment on attachment 388467 [details] [diff] [review]
First cut, still needs sandbox testing.  Fixed SpinLockSymbian line endings.

r- only because the change to WinPortUtils is unacceptable without further explanation (you remove the comment that explained why that code was the way it is, and there were two instances in the code of this pattern - the other is MMgcPortWin.cpp:491).  Otherwise this looks OK.

Comment 5

[Tommy Reilly]

The comment referred to a warning about constructing an object and I removed the object so the comment was obsolete.   Let me if you think I should add some comment back in....

Comment 6

[Lars T Hansen]

Comment on attachment 388467 [details] [diff] [review]
First cut, still needs sandbox testing.  Fixed SpinLockSymbian line endings.


If you've tested it in the Player and there's no warning then I'm fine with it, but then please fix MMgcPortWin.cpp too :-)

Comment 7

[Tommy Reilly]

Pushed
changeset:   2138:e5f4ee8c714a

Comment 8

[Tommy Reilly]

Attachment: Fix lock, make sure to use proper lock declaration

Extra change required to get solaris to pass, can't assume all locks are void * and each platform needs its own vmpi_spin_lock_t def.

Comment 9

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.