555760 - Extend VMPI to cover thread creation, mutexes, condition vars and atomics

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, enhancement, P3)

Description

[Simon Wilkinson]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: 


The small amount of code that is currently multi-threaded is written in a platform-dependent manner.

This will become unmanageable as the VM becomes more multi-threaded. A more attractive solution is to have a low-level thread porting layer (which looks similar to pthreads/OSAtomic), from which higher-level C++ abstractions can be built (monitors, barriers etc.)

The attached patch is an extension to VMPI that defines a common interface and platform implementations for thread creation, mutexes, condition variables and atomics:

void VMPI_memoryBarrier();
int32_t VMPI_atomicIncAndGet32(int32_t* address);
int32_t VMPI_atomicIncAndGet32WithBarrier(int32_t* address);
bool VMPI_threadCreate(vmpi_thread_t* thread, vmpi_thread_start_t start_fn, vmpi_thread_arg_t arg);
bool VMPI_threadDetach(vmpi_thread_t thread);
void VMPI_threadSleep(int32_t timeout_millis);
void VMPI_threadJoin(vmpi_thread_t thread);
bool VMPI_recursiveMutexInit(vmpi_mutex_t* mutex);
bool VMPI_recursiveMutexDestroy(vmpi_mutex_t* mutex);
void VMPI_recursiveMutexLock(vmpi_mutex_t* mutex);
void VMPI_recursiveMutexUnlock(vmpi_mutex_t* mutex);
bool VMPI_condVarInit(vmpi_condvar_t* condvar);
bool VMPI_condVarDestroy(vmpi_condvar_t* condvar);
void VMPI_condVarWait(vmpi_condvar_t* condvar, vmpi_mutex_t* mutex);
bool VMPI_condVarTimedWait(vmpi_condvar_t* condvar, vmpi_mutex_t* mutex, int32_t timeout_millis);
void VMPI_condVarBroadcast(vmpi_condvar_t* condvar);
void VMPI_condVarSignal(vmpi_condvar_t* condvar);

There are a few options for implementing Condition variables on Windows. We can discuss this.

Symbian atomics are left unimplemented. Can someone advise?


Reproducible: Always

Comment 1

[Simon Wilkinson]

Attachment: Initial patch

Comment 2

[Stan Switzer]

See FP win32utils.cpp for a lightweight, unobvious but well-worn-in condition variable implementation based on QueueUserAPC().

You might also want to look at the FP Monitor.h for a Java-like monitor abstraction that hides locked variables except when locked. Contract enforcement FTW!

And also it's a very good idea to build deadlock detection logic into the lock primitives in debug builds. Look at the FP impls as well.

Comment 3

[Simon Wilkinson]

(In reply to comment #2)
> See FP win32utils.cpp for a lightweight, unobvious but well-worn-in condition
> variable implementation based on QueueUserAPC().

I actually based my implementation on this.
Initially I was going to go for a boost-like algorithm (Alexander Terekhov's). But the win32utils implementation pointed the way to not being overly-concerned about absolutely fair scheduling to notified threads.

> You might also want to look at the FP Monitor.h for a Java-like monitor
> abstraction that hides locked variables except when locked. Contract
> enforcement FTW!

I have WaitNotifyMonitor in bug 555765, which builds C++ abstraction on top of the VMPI extensions. I'll take a look at the Monitor.h version. Thanks!

> And also it's a very good idea to build deadlock detection logic into the lock
> primitives in debug builds. Look at the FP impls as well.

This is something I wanted to add to bug 555765 at some point, but as some others have mentioned, it would be a good thing to have only a single implementation of all this stuff.

Comment 4

[Stan Switzer]

(In reply to comment #3)
> > You might also want to look at the FP Monitor.h for a Java-like monitor
> > abstraction that hides locked variables except when locked. Contract
> > enforcement FTW!
> 
> I have WaitNotifyMonitor in bug 555765, which builds C++ abstraction on top of
> the VMPI extensions. I'll take a look at the Monitor.h version. Thanks!
> 
> > And also it's a very good idea to build deadlock detection logic into the lock
> > primitives in debug builds. Look at the FP impls as well.
> 
> This is something I wanted to add to bug 555765 at some point, but as some
> others have mentioned, it would be a good thing to have only a single
> implementation of all this stuff.

Making more of that common would be very good; I remember there was a complication but probably not a serious one.

I want to point out that _sharing_ the deadlock detection between the AvmCore and its clients will catch a broader class of problems than having parallel but separate ones. So let's be sure we find a way to share it if/when you do this.

Comment 5

[Simon Wilkinson]

Attachment: Rebased, added unix atomics, added missing volatiles

- Rebased to latest version.
- Added inline asm implementations of atomic primitives for Unix.
- Added various missing volatile qualifiers

Comment 6

[Stan Switzer]

I was looking at (possibly not the latest) version of windows spinlocks and a couple of comments:

VMPI_lockAcquire should probably should use SwitchToThread rather than Sleep (but maybe not on CE).

Also, typically a spin-lock would check if it's on an MP machine and if so briefly spin in a loop before yielding to the OS. When spin locks are being used correctly (for very brief spans of code) this will be much much faster. You needn't worry overmuch about pipeline stalls from the tests; after all wasting time is pretty much the whole point.

Comment 7

[Simon Wilkinson]

(In reply to comment #6)
> I was looking at (possibly not the latest) version of windows spinlocks and a
> couple of comments:
> 
> VMPI_lockAcquire should probably should use SwitchToThread rather than Sleep
> (but maybe not on CE).
> 
> Also, typically a spin-lock would check if it's on an MP machine and if so
> briefly spin in a loop before yielding to the OS. When spin locks are being
> used correctly (for very brief spans of code) this will be much much faster.
> You needn't worry overmuch about pipeline stalls from the tests; after all
> wasting time is pretty much the whole point.

Opened up bug 562687 for this one.

Comment 8

[Simon Wilkinson]

Attachment: latest

Added VMPI_recursiveMutexTryLock.

Fixed atomics for Solaris, Android and WinCE.

Comment 9

[Stan Switzer]

I'm looking at some of our thread and sync primitives to see what it would take to consolidate on these primitives. In addition to what you show, we use the following:

 - thread create has low/normal/high priority
 - there is a way to get and compare thread IDs (so that you can assert you are on the intended thread)

(People tend to assume that you can compare pthread_t with ==, but then what is pthread_equal() for? Standards can be so aggravating that way!)

Presumably VMPI_threadCreate reserves "big enough" stack and guard areas. But really... how does it know?

You might want a "yield" abstraction to address the problem that win32 SwitchToThread addresses.  I'm wondering what VMPI_threadSleep is intended for, but something like a "yield" would be better than sleep() in a spin-lock implementation.

Finally, to the extent these primitives are clearly modelled on POSIX (yay!), shouldn't time be represented in nanoseconds?  A millisecond is, like, _forever_.

Comment 10

[Simon Wilkinson]

(In reply to comment #9)
> I'm looking at some of our thread and sync primitives to see what it would take
> to consolidate on these primitives. In addition to what you show, we use the
> following:
> 
>  - thread create has low/normal/high priority

This would actually be quite useful for the VM (e.g. lower-priority compiler threads).
A pthread_attr_ type of interface is the natural choice. This can cover your requirement for stack/guard sizing too. 


>  - there is a way to get and compare thread IDs (so that you can assert you are
> on the intended thread)
> 
> (People tend to assume that you can compare pthread_t with ==, but then what is
> pthread_equal() for? Standards can be so aggravating that way!)


Is the current vmpi_thread_t ID not sufficient?

> Presumably VMPI_threadCreate reserves "big enough" stack and guard areas. But
> really... how does it know?

See the pthread_attr_ comment above.

> You might want a "yield" abstraction to address the problem that win32
> SwitchToThread addresses.  I'm wondering what VMPI_threadSleep is intended for,
> but something like a "yield" would be better than sleep() in a spin-lock
> implementation.

A efficient yield is on my to-do list.

> Finally, to the extent these primitives are clearly modelled on POSIX (yay!),
> shouldn't time be represented in nanoseconds?  A millisecond is, like,
> _forever_.

I'd say it's about 90% POSIX flavored. That was one of my win32 compromises. May be a bad one!

Comment 11

[Simon Wilkinson]

Attachment: Latest (with full WinCE support)

WinCE builds now use the condition variable implementation from bug 574695

Comment 12

[Stan Switzer]

> >  - thread create has low/normal/high priority
> ...
> A pthread_attr_ type of interface is the natural choice. This can cover your
> requirement for stack/guard sizing too. 

Yeah, I guess.  It's sorta chatty but nicely extensible. The pthread init/set/destroy drill is tedious and also somewhat dangerous since some of those calls can be no-ops on some platforms. That is: it could work perfectly fine on some platforms but have subtle problems on others.

> >  - there is a way to get and compare thread IDs (so that you can assert you are
> > on the intended thread)
> > 
> > (People tend to assume that you can compare pthread_t with ==, but then what is
> > pthread_equal() for? Standards can be so aggravating that way!)
> 
> 
> Is the current vmpi_thread_t ID not sufficient?

Not sure; you may need a thread-id-compare primitive. As I was saying, == seems to work just fine in practice, but POSIX apparently doesn't guarantee it. I suppose that in a pinch a platform could define vmpi_thread_t as a class with an overload ==, if it ever came to that.

> > Finally, to the extent these primitives are clearly modelled on POSIX (yay!),
> > shouldn't time be represented in nanoseconds?  A millisecond is, like,
> > _forever_.
> 
> I'd say it's about 90% POSIX flavored. That was one of my win32 compromises.
> May be a bad one!

Maybe define it in nanoseconds with the caveat that precision and accuracy may be wildly different. But then you might need to define a constant for the granularity or the smallest value that isn't rounded down to zero.  BTW, how would you round it?  POSIX is apparently mute on this but a fair argument can be made that you'd have to round up (at least for values that would truncate to zero): otherwise there is no safe small wait constant that couldn't always result in ETIMEDOUT, and that seems wrong.

Comment 13

[Simon Wilkinson]

Attachment: Added thread startup attributes

This new revision addresses some of Stan's requirements in aligning this work with the current FP implementation.

Added:
- Specify thread priority (low/normal/high) at thread startup.
- Specify stack size at thread startup.
- Specify guard area size at thread startup.

The API looks like pthread_attr_*, but is much smaller and doesn't include the unnecessary get* functions. The idea is that higher-level C++ thread abstractions will hide the opaque pointer handling (e.g. see VMThread::start(ThreadPriority priority) in bug 555765)

Clearly all of our platforms don't support all of the options. Summarized below:

              pthread     win32
priority        no         yes
stack size      yes        yes
guard size      yes        no (possible starting with Vista)


One unresolved question: can you find out the default stack size of a win32 process programmatically? (i.e. the value set by the linker)
At the moment I just guess it's going to be the 1MB default.

Comment 14

[Lars T Hansen]

Re Symbian, adding Samuli for possible comment on the issue.

Re Win32 stack size, I have a vague memory from somewhere that the default stack size is 512KB but that seems laughably small.  We do have VMPI functionality to query what it actually is (or at least that code can be adapted to suit your needs), will that not work properly for you?

Comment 15

[Samuli Kekki]

On Symbian you set the stack size at compile time. We can return the same hard coded value from a VMPI method as needed. 80k is the max size on Symbian OS as of today.

Whether all the other proposed APIs are possible to implement on Symbian would require some investigation.

Comment 16

[Simon Wilkinson]

(In reply to comment #14)
> Re Win32 stack size, I have a vague memory from somewhere that the default
> stack size is 512KB but that seems laughably small.  We do have VMPI
> functionality to query what it actually is (or at least that code can be
> adapted to suit your needs), will that not work properly for you?

The default stack is 1MB: http://msdn.microsoft.com/en-us/library/8cxs58a6%28vs.71%29.aspx.

But it can be changed at link time per application.

However, I can't find out what the linker set the default to programmatically from within the application. I.e. I can't find an equivalent of:

size_t getThisApplicationsDefaultStackSize()
{
    pthread_attr_t attr;
    size_t size;
    pthread_attr_init(&attr);
    pthread_attr_getstacksize(&attr, &size);
    return size;
}

Do you mean that there is somewhere in VMPI that has solved this for win32?

Comment 17

[Simon Wilkinson]

(In reply to comment #15)
> On Symbian you set the stack size at compile time. We can return the same hard
> coded value from a VMPI method as needed. 80k is the max size on Symbian OS as
> of today.
> 
> Whether all the other proposed APIs are possible to implement on Symbian would
> require some investigation.

I've done some investigations. I think the only difficult part it support for atomic primitives. Although a compare-and-swap is all we need to emulate everything else.

Comment 18

[Simon Wilkinson]

Attachment: Added further atomic primitives

Added addition atomic operations.
Implemented for all platforms apart from Symbian and Android.

Added:

int32_t VMPI_atomicDecAndGet32(volatile int32_t* value);
int32_t VMPI_atomicDecAndGet32WithBarrier(volatile int32_t* value);
bool VMPI_compareAndSwap32(int32_t oldValue, int32_t newValue, volatile int32_t* address);
bool VMPI_compareAndSwap32WithBarrier(int32_t oldValue, int32_t newValue, volatile int32_t* address);
void VMPI_memoryBarrier();

Comment 19

[Stan Switzer]

(In reply to comment #13)
>
> Clearly all of our platforms don't support all of the options. Summarized
> below:
> 
>               pthread     win32
> priority        no         yes
> stack size      yes        yes
> guard size      yes        no (possible starting with Vista)

pthread_attr_setschedparam appears to allow you to control priorities but I gather that is for realtime scheduling modes we wouldn't typically use.

A fair case can be made that we don't actually need those priorities and can just get rid of them.  OS X platform code (and perhaps others) ignores the parameter anyway. Of the uses of anything other than normal priority, only the microphone code seems to be of any significance. No current use of low priority seems particularly well-motivated. Probably should ask turo.

My suggestion would be something like this: the VMPI thread creation primitive has NO priority param priority and then we provide a separate primitive (perhaps NOT in VMPI) to allow a spawned thread to raise its own priority (if possible).  That way VMPI can have a sufficient, well-defined mechanism for its purposes, and client code can goose the priority of threads when necessary and possible (microphone case).

Comment 20

[Stan Switzer]

(In reply to comment #19)
> My suggestion would be something like...

Or not.  I suppose having a priority param that's honored if possible is a fine solution as well. I was looking for an alternative that allowed VMPI to push the vague stuff to the client layer, but you don't have to care about that if you don't want to.  :)

Comment 21

[Lars T Hansen]

(In reply to comment #16)

> Do you mean that there is somewhere in VMPI that has solved this for win32?

Follow the calls from ShellCoreImpl::setStackLimit - I believe this is correct for both Win32 and pthreads, though it may be more limited than you need.  I misremembered its location - it's in the shell code, and not in VMPI, because the Flash Player has its own code for this functionality.

Comment 22

[Simon Wilkinson]

(In reply to comment #20)
> My suggestion would be something like this: the VMPI thread creation primitive
> has NO priority param priority and then we provide a separate primitive
> (perhaps NOT in VMPI) to allow a spawned thread to raise its own priority (if
> possible).  That way VMPI can have a sufficient, well-defined mechanism for its
> purposes, and client code can goose the priority of threads when necessary and
> possible (microphone case).

My original plan was to not have any platform-specific threading implementation above VMPI. So VMPI must provide the priority API if one is going to exist at all, even if it is eventually wrapped by higher-level thread abstractions.

I think it's totally acceptable to have the priorities ignored if the platform cannot honor them.

Comment 23

[Simon Wilkinson]

Attachment: Added

Comment 24

[Simon Wilkinson]

Attachment: Latest. All platforms implemented.

Added in this version:

- Atomic operations for Solaris
- Atomic operations for Unix (a GCC intrinsics version if available, an x86 asm version, and a fallback mutex-based emulation for ARM)
- Atomic operations for Symbian (currently uses the same mutex-based emulation as Unix/ARM)
- WinCE cmdline build links win32cv from bug 574695
- Lots of trivial platform-specific fixes to satisfy the buildbot

Comment 25

[Simon Wilkinson]

Attachment: Selftest for VMPI threads

Added a set of selftests for VMPI threads, mutexes, conditions and atomics.

These are available via -Dselftest=vmpi,threads.

Updated the list of smokes to run the above selftests.

Comment 26

[Simon Wilkinson]

Attachment: Manifests for WinCE with win32cv included

Comment 27

[Simon Wilkinson]

Here's some outstanding issues that merit discussion:

- Do we want wait/sleep times to be nanos, not millis.

- Possible addition of a VMPI_threadYield(), who's goal is to minimize priority inversion.

- An API for comparing the identity of threads. Currently, vmpi_thread_t's are just compared with ==, which looks to be ok.

- Efficient atomics implementation for Symbian and Android (some API's are available). Also fallback asm implementation for ARM. Currently this is emulated with pthread mutexes.

Comment 28

[Simon Wilkinson]

(In reply to comment #27)
> Here's some outstanding issues that merit discussion:

One more thing:

- Should the atomics section of the API be maximal (i.e. have lots primitives like OSX's OSAtomics), or minimal (i.e. just have CAS and barrier, and build everything else in higher-level libraries, e.g. bug 555765's AtomicOps::or32)

Comment 29

[Krzysztof Palacz]

Comment on attachment 473967 [details] [diff] [review]
Latest. All platforms implemented.

Nothing seems obviously wrong to me, as the saying goes.
We have GC memory barriers, so we could refer to the ordering instructions as fences, but the probability of confusion is probably very low.

Comment 30

[Lars T Hansen]

Comment on attachment 473967 [details] [diff] [review]
Latest. All platforms implemented.

Overall this seems nice.  (Not a subject where I have deep expertise.)

Nits:

I don't understand what 'rnt' stands for in 'vmpi_thread_rnt_t'; a comment or (almost always better anyway) a non-abbreviated name might constitute an improvement.

TAB characters in ThreadsWin.cpp.

Outstanding FIXME in ThreadsWin.cpp.

Do you want dummyAPC do be static to hide it from view?

In all instances where a comment says "Note that on some platforms ..." I would strongly encourage you to write down (in VMPI.h) /which/ platforms that you know about where this is an issue; it aids debugging when somebody trips over the bug, and is valuable information to keep around in any case.  Yes, the information can go stale and yes it may need to be updated, but something is much better than nothing.

I assume this warrants a FIXME (in unix-platform.h), given the imporance of ARM:

  +#ifdef AVMSYSTEM_ARM
  +
  +#define EMULATE_ATOMICS_WITH_PTHREAD_MUTEX

Comment 31

[Stan Switzer]

(In reply to comment #28)
> (In reply to comment #27)
> > Here's some outstanding issues that merit discussion:
> 
> One more thing:
> 
> - Should the atomics section of the API be maximal (i.e. have lots primitives
> like OSX's OSAtomics), or minimal (i.e. just have CAS and barrier, and build
> everything else in higher-level libraries, e.g. bug 555765's AtomicOps::or32)

Well IMO it's probably better to restrict ourselves to the things that can be implemented efficiently "everywhere." Because if, for instance, people assume that AtomicOps::or32 is primitive-efficient, people might use that in preference to a CAS-based solution which might be nearly as fast as a primitive or32-based solution but a lot faster than a software atomic or32 implementation.

Or alternatively, they exist only when they're genuinely primitive and there's an ifdef telling the implementation whether to try to use them.

Anyway, it seems to me that software emulation of atomic primitives has the potential to do more harm than good.

Comment 32

[Stan Switzer]

(In reply to comment #31)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > Here's some outstanding issues that merit discussion:
> > 
> > One more thing:
> > 
> > - Should the atomics section of the API be maximal (i.e. have lots primitives
> > like OSX's OSAtomics), or minimal (i.e. just have CAS and barrier, and build
> > everything else in higher-level libraries, e.g. bug 555765's AtomicOps::or32)
> 
> Well IMO it's probably better to restrict ourselves to the things that can be
> implemented efficiently "everywhere." Because if, for instance, people assume
> that AtomicOps::or32 is primitive-efficient, people might use that in
> preference to a CAS-based solution which might be nearly as fast as a primitive
> or32-based solution but a lot faster than a software atomic or32
> implementation.
> 
> Or alternatively, they exist only when they're genuinely primitive and there's
> an ifdef telling the implementation whether to try to use them.
> 
> Anyway, it seems to me that software emulation of atomic primitives has the
> potential to do more harm than good.

Meh, never mind, shouldn't reply before coffee kicks in. :) A CAS-based emulation would be trivial so the point is moot.  Still, all things being equal, minimal primitives reduce the porting and testing burdens.

Comment 33

[Stan Switzer]

Comment on attachment 473967 [details] [diff] [review]
Latest. All platforms implemented.

stan: feedback+

Comment 34

[Dan Smith]

Removed Stan from the feedback list as he can't r+ directly there without higher privs.  See comment 33 for his r+.

Comment 35

[Edwin Smith]

Comment on attachment 473967 [details] [diff] [review]
Latest. All platforms implemented.

Nothing major to report, just a few questions for Simon/Krzystof to resolve.  patch applied cleanly and rebased cleanly, even though it is old.

pinging K. for selftest review.  Without a rationale for less, it should get to 100% line coverage for the new code.  if this cant be done before landing then followup bugs should be created.

VMPI.h
> Note that it is platform dependent if non-default attributes are honored or completely ignored.

presumably this is okay if all attributes are hints, but if any are counted on by a client for correctness, and known to be non-portable, then should we assert if they are set and run on an unsupported client?

> After a thread has been detached, any attempt to join the
> thread has undefined behavior.
> Multiple detachments of the same thread results in undefined behavior.

can we detect these and assert?  (similar for other undefined behaviors as I keep reading on... i wont list them all. essentially, I'm talking about asserts for all api invariants that are reasonably detectable).


> // FIXME: Where do we define _WIN32_WINNT as _WIN32_WINNT_WINXP so that VC++ picks up the correct THREAD_ALL_ACCESS?

Resolve, or create a bug and put bug# in comment, for example:

  // FIXME: bug XXX Where do we ...

In ThreadsWin.cpp:

  size_t VMPI_threadAttrDefaultStackSize()
  {
    // Stack size is set in the linker. The default is 1MB:
    // http://msdn.microsoft.com/en-us/library/8cxs58a6%28vs.71%29.aspx
    // But can we find this out programmatically? There doesn't appear to be an API:
    // http://msdn.microsoft.com/en-us/library/ms686774%28v=VS.85%29.aspx
    return 1024 * 1024;
  }

This probably needs a FIXME and bug#, depending on the likelihood and
penalty for being wrong (crash!).  I didn't look at other platforms but one bug to track the work is enough.

(nit) ThreadsWin.cpp - stray #endifs should have a // WHATEVER saying what they match with.  inconsistent blank lines between functions.  maybe elsewhere, but this is where I noticed it.

win32-platform.h, in VMPI_memoryBarrier():
> // This is a temp solution until we have a x86 asm version.

worth a fixme bug?

Comment 36

[Krzysztof Palacz]

Comment on attachment 473972 [details] [diff] [review]
Selftest for VMPI threads

Looks good to me, minor nits:
Don't we need to update the vcproj files as well?
m_sharedCounter is public, I think it shouldn't have the m_ prefix.
Ifdefing that 100000 would help understanding the code.
There's no need to typedef structs in C++ (ThreadRecord).

Comment 37

[Simon Wilkinson]

Attachment: Post-review changes

Comment 38

[Simon Wilkinson]

Attachment: Latest selftests. API fully covered apart from MemoryBarriers

Comment 39

[Simon Wilkinson]

Attachment: Main patch. Post-review changes

Comment 40

[Simon Wilkinson]

(In reply to comment #30)
> Comment on attachment 473967 [details] [diff] [review]
> Latest. All platforms implemented.
> 
> Overall this seems nice.  (Not a subject where I have deep expertise.)
> 
> Nits:
> 
> I don't understand what 'rnt' stands for in 'vmpi_thread_rnt_t'; a comment or
> (almost always better anyway) a non-abbreviated name might constitute an
> improvement.

Changed to be rtn, i.e. return. I've made a comment as to what its intended meaning is. 

> TAB characters in ThreadsWin.cpp.
> 
> Outstanding FIXME in ThreadsWin.cpp.

Fixed.

> Do you want dummyAPC do be static to hide it from view?

Yes. Commented.

> In all instances where a comment says "Note that on some platforms ..." I would
> strongly encourage you to write down (in VMPI.h) /which/ platforms that you
> know about where this is an issue; it aids debugging when somebody trips over
> the bug, and is valuable information to keep around in any case.  Yes, the
> information can go stale and yes it may need to be updated, but something is
> much better than nothing.

I've added comments where appropriate.

> I assume this warrants a FIXME (in unix-platform.h), given the imporance of
> ARM:
> 
>   +#ifdef AVMSYSTEM_ARM
>   +
>   +#define EMULATE_ATOMICS_WITH_PTHREAD_MUTEX

See bug 609809 (Android) and bug 609810 (other ARM)

Comment 41

[Simon Wilkinson]

(In reply to comment #35)
> Comment on attachment 473967 [details] [diff] [review]
> Latest. All platforms implemented.
> 
> Nothing major to report, just a few questions for Simon/Krzystof to resolve. 
> patch applied cleanly and rebased cleanly, even though it is old.

The new patches are rebased to the tr tip.

> pinging K. for selftest review.  Without a rationale for less, it should get to
> 100% line coverage for the new code.  if this cant be done before landing then
> followup bugs should be created.

I've added more coverage to the selftests. They should now cover the whole API apart from memory barrier functions. I've actually included a memory barrier selftest but turned it off until I'm more confident in its results. See bug 609943.

> VMPI.h
> > Note that it is platform dependent if non-default attributes are honored or completely ignored.
> 
> presumably this is okay if all attributes are hints, but if any are counted on
> by a client for correctness, and known to be non-portable, then should we
> assert if they are set and run on an unsupported client?

I've documented which attributes are supported per platform:
               pthread     win32 (XP version APIs)
priority        no         yes
stack size      yes        yes
guard size      yes        no

Priority will only ever be a hint, there are no correctness issues. Should we just remove guard size from the attributes?


> > After a thread has been detached, any attempt to join the
> > thread has undefined behavior.
> > Multiple detachments of the same thread results in undefined behavior.
> 
> can we detect these and assert?  (similar for other undefined behaviors as I
> keep reading on... i wont list them all. essentially, I'm talking about asserts
> for all api invariants that are reasonably detectable).

I was going to put all the debug code within the C++ abstractions, one layer up. Clients should not be using these APIs directly.

> > // FIXME: Where do we define _WIN32_WINNT as _WIN32_WINNT_WINXP so that VC++ picks up the correct THREAD_ALL_ACCESS?
> 
> Resolve, or create a bug and put bug# in comment, for example:
> 
>   // FIXME: bug XXX Where do we ...

Fixed. Should not matter for shell builds. Player builds will set the value correctly.


> In ThreadsWin.cpp:
> 
>   size_t VMPI_threadAttrDefaultStackSize()
>   {
>     // Stack size is set in the linker. The default is 1MB:
>     // http://msdn.microsoft.com/en-us/library/8cxs58a6%28vs.71%29.aspx
>     // But can we find this out programmatically? There doesn't appear to be an
> API:
>     // http://msdn.microsoft.com/en-us/library/ms686774%28v=VS.85%29.aspx
>     return 1024 * 1024;
>   }
> 
> This probably needs a FIXME and bug#, depending on the likelihood and
> penalty for being wrong (crash!).  I didn't look at other platforms but one bug
> to track the work is enough.

Opened bug 609822

> (nit) ThreadsWin.cpp - stray #endifs should have a // WHATEVER saying what they
> match with.  inconsistent blank lines between functions.  maybe elsewhere, but
> this is where I noticed it.

Fixed.

> win32-platform.h, in VMPI_memoryBarrier():
> > // This is a temp solution until we have a x86 asm version.
> 
> worth a fixme bug?

Yes.
See bug 609820

Comment 42

[Simon Wilkinson]

(In reply to comment #36)
> Comment on attachment 473972 [details] [diff] [review]
> Selftest for VMPI threads
> 
> Looks good to me, minor nits:
> Don't we need to update the vcproj files as well?

Done in the latest patch.

> m_sharedCounter is public, I think it shouldn't have the m_ prefix.

Fixed.

> Ifdefing that 100000 would help understanding the code.

#defined as ITERATIONS

> There's no need to typedef structs in C++ (ThreadRecord).

Oops. Found this in few other places too.

Comment 43

[Simon Wilkinson]

Attachment: Latest selftests

Comment 44

[Simon Wilkinson]

Attachment: Main patch. Post-review changes and win32 fixes

Comment 45

[Simon Wilkinson]

Attachment: Latest selftests

Comment 46

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 488682 [details] [diff] [review]
Main patch. Post-review changes and win32 fixes

typo in VMPI.h: "no memory barrier we be applied." (this typo appears at least twice in the patch.).

(I got distracted before I could finish going over the patch.)

Comment 47

[Krzysztof Palacz]

Fixed typos and pushed:
http://hg.mozilla.org/tamarin-redux/rev/8983e7687b85

Comment 48

[Felix S. Klock II [:pnkfelix, :fklock]]

Augmented previous push by throwing in the missing ThreadsPosix-inlines.h file.

TR 5526:3748b03ad186
http://hg.mozilla.org/tamarin-redux/rev/3748b03ad186

Comment 49

[Simon Wilkinson]

Attachment: Latest selftests. Added conservative memory barrier.

Comment 50

[Krzysztof Palacz]

Comment on attachment 489422 [details] [diff] [review]
Latest selftests. Added conservative memory barrier.

minor nit: 
I'd move the explantation of what MemoryBarrierTest does next to its implementation.

Comment 51

[Simon Wilkinson]

(In reply to comment #50)
> Comment on attachment 489422 [details] [diff] [review]
> Latest selftests. Added conservative memory barrier.
> 
> minor nit: 
> I'd move the explantation of what MemoryBarrierTest does next to its
> implementation.

I'll note this here, as well as in the code:
The memory barrier test is turned off pending bug 609943

Comment 52

[Edwin Smith]

Comment on attachment 489422 [details] [diff] [review]
Latest selftests. Added conservative memory barrier.

It looks like something weird happened with line endings in class MemoryBarrierTest

Comment 53

[Simon Wilkinson]

Attachment: Latest selftests. Added docs. Fixed spacing.

Comment 54

[Krzysztof Palacz]

Pushed selftests
http://hg.mozilla.org/tamarin-redux/rev/b2dbe070c5bc

Comment 55

[Felix S. Klock II [:pnkfelix, :fklock]]

Simon: can this be closed?

Comment 56

[Simon Wilkinson]

(In reply to comment #55)
> Simon: can this be closed?

Yes it can be closed.

Question on protocol: is it me or my landing buddy who should set this status? Previously it has been my landing buddy...