555765 - Cross-platform C++ concurrency abstractions built from VMPI extensions

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: 

Cross-platform C++ concurrency abstractions built from VMPI extensions

Reproducible: Always

Comment 1

[Simon Wilkinson]

Attachment: Initial patch

Comment 2

[Simon Wilkinson]

The classes can be split into 2 groups: Synchronization and Threads.

Synchronization Classes:

- RecursiveMutex - OO-wrapper for a mutex, with RAII-style mutex init/destroy.

- CondVar - OO-wrapper for a condition variable, with RAII-style condvar init/destroy.

- WaitNotifyMonitor - implements a Java-style monitor.

- ScopedLocker - provides RAII-style locking for WaitNotifyMonitors.

- CyclicBarrier - a reusable barrier implemented via WaitNotifyMonitor.

- VMAtomic - a collection of atomic and fence operations.


Thread Classes:

- VMThreadLocal - OO-wrapper for the native TLS implementation

- VMThread - Simple object-oriented thread abstraction. Calling start() on a VMThread instance creates a new native thread. The thread begins execution from the run() method of a Runnable object, which can be passed to the VMThread constructor. If no Runnable is passed in the constructor, the VMThread calls its own run() method, which can be overridden. Apart from providing the convenience of an OO-thread abstraction, VMThread provides some extras such as thread-safe joins with other VMThreads (which pthreads does not). Note that, by design, a VMThread instance does not depend on the presence of, or have any affinity with, any AvmCore instance.

Comment 3

[Steven Johnson]

You may want to look at the "minithreads" functionality in Flash for comparison, since it is also a cross-platform wrapper around thread functionality. (If we model it properly then we could eventually have minithreads as a trivial wrapper around the above)

Comment 4

[Simon Wilkinson]

(In reply to comment #3)
> You may want to look at the "minithreads" functionality in Flash for
> comparison, since it is also a cross-platform wrapper around thread
> functionality. (If we model it properly then we could eventually have
> minithreads as a trivial wrapper around the above)

I did take a look at it previously; it's where I got my cues for the VMPI implementation of Condition Variables on win32 (i.e. don't worry about completely fair scheduling for notified threads).

Comment 5

[Simon Wilkinson]

Attachment: Rebased. Added AtomicCounter class

- Rebased to latest version
- Replaced procedural atomic integer API with AtomicCounter class.

Comment 6

[Simon Wilkinson]

Attachment: latest

Added:
- Safepoint support at blocking calls.
- New scoped-locking macros.
- Start of debug API.
- tryLock to RecursiveMutex and WaitNotifyMonitor.

Comment 7

[Simon Wilkinson]

Attachment: Latest with auto-unlock-and-rethrow Exception support

There are now 2 types of RAII-lockers available for WaitNotifyMonitors: the original version that is not Exception aware, and a new version that will automatically unlock and then rethrow any exception that is thrown out of the critical section. (Clearly this refers to the AVM's exception mechanism, not the unused C++ one).

The general form of the RAII-locking macros are:

SCOPE_LOCK(monitor) {
    // Critical section
}

and

SCOPE_LOCK_RETHROW(monitor, core) {
    // Critical section.
    // Any exceptions thrown out of the critical section will
    // unlock the monitor.
}


Note the macros' sugaring into Java/C# style synchronized-blocks, which I find much more readable than plain RAII or procedural locking.

There are also Safepoint-aware version of these constructs in bug 575544

Comment 8

[Stan Switzer]

The more I think about this the less I like it.  

1) Throwing exceptions through a critical section violates its transaction-nature.
2) These aren't "real" C++ exceptions but our actionscript exceptions.
3) Why would we need to or want to throw ActionScript exceptions through a C++ critical section?

The use case seems dubious.  Far better to just consider that an error and have just one variety that does not rethrow and which--in in debug builds--asserts if that very unexpected thing ever happens.

I think the whole idea of rethrowing from an critical section just belabors a situation that should never happen and for which I doubt we can find any compelling use cases.

Even the argument of "leaving locks unexpectedly locked" doesn't hold up.  Exiting an "atomic" block unexpectedly is worse and in fact far less likely to be discovered.  I'll bet you'd notice if a lock was left locked.  You might not discover inconsistent state so easily.

Comment 9

[Simon Wilkinson]

(In reply to comment #8)
> The more I think about this the less I like it.  
> 
> 1) Throwing exceptions through a critical section violates its
> transaction-nature.

Transactional semantics are the burden of the programmer, no matter what sort of early exit you make from a critical section. Violations will happen if you fail to manually roll-back any necessary shared state. It is a separate question if the subsequent unlock-and-rethrow is sugared into the RAII locker. For example, where now you could do this:

Locker locker;
TRY (core(), kCatchAction_Rethrow) {
    if (!bar() {
        undo_bar(); // roll-back
        return;
    }
    foo(); // throws exception
}      
CATCH(Exception *exception) {
	undo_foo(); // roll-back
    locker.unlock(); // RAII locker will fail, so must unlock manually
    core()->throwException(exception);
}

With the new constructs you don't have to short-cut the RAII locker, and it end's up working just like "real" C++ exceptions.

SCOPE_LOCK_RETHROW(monitor, core()) {
    TRY (core(), kCatchAction_Rethrow) {
        if (!bar() {
            undo_bar(); // roll-back
            return;
        }
        foo(); // throws exception
    }      
    CATCH(Exception *exception) {
    	undo_foo(); // roll-back
        core()->throwException(exception);
    }
}


> 2) These aren't "real" C++ exceptions but our actionscript exceptions.
> 3) Why would we need to or want to throw ActionScript exceptions through a C++
> critical section?

I was considering them 'lighter-weight' C++ exceptions, that can hold a reference to an ActionScript error object if necessary.


> The use case seems dubious.  Far better to just consider that an error and have
> just one variety that does not rethrow and which--in in debug builds--asserts
> if that very unexpected thing ever happens.
> 
> I think the whole idea of rethrowing from an critical section just belabors a
> situation that should never happen and for which I doubt we can find any
> compelling use cases.
> 
> Even the argument of "leaving locks unexpectedly locked" doesn't hold up. 
> Exiting an "atomic" block unexpectedly is worse and in fact far less likely to
> be discovered.  I'll bet you'd notice if a lock was left locked.  You might not
> discover inconsistent state so easily.

It's true that for a language without explicit concurrency, which is executed by a single-threaded VM implementation, there is no immediate need to start throwing exceptions through critical sections on a regular basis. I doubt this will be the case for much longer though.
The main motivation for adding this came from Krzysztof's experiments with headless worker VM's, where unlock-and-rethrow was apparently a common pattern.

Comment 10

[Simon Wilkinson]

Attachment: latest with thread priorites, stack and guard size options

Supplemented the thread spawning method:

bool VMThread::start();

With:

bool VMThread::start(ThreadPriority priority);
bool VMThread::start(ThreadPriority priority, size_t stackSize, size_t guardSize);

These hide the necessary quirks of the lower-level VMPI_threadAttr* API

Comment 11

[Stan Switzer]

Well RIAA-everything fails with AS exceptions, so RIAA exceptions aren't so different,.

However if there is a catch and rethrow use case then perhaps integrating the two together so that you have a lock-with-catch-block notation.  Then at least people won't get the very very wrong idea that they should prefer SCOPE_LOCK_RETHROW "just to be safe."  Because that is NOT safe.

To be safe, they need to undo their transaction AND unlock the lock.  So maybe a construct that tied that all together would be good and it has the added benefit that you don't end up having two nested try-catch blocke in the implementation.

Something rather like (but whatever, as long as the catch is mandatory):

SCOPE_LOCK_RETHROW(monitor, core()) {
   stuff.
|
CATCH(Exception *exception) {
   undo_stuff
   core()->throwException(exception);
}

I feel the greater evil is to accidentally leave unrolled-backstate (RIAA will fail here too!) than to leave locks locked.  You WILL discover you've left locks locked.  Inconsistent state might be a far more insidious bug. Seems a shame to make it convenient for people to do that.

Comment 12

[Simon Wilkinson]

Attachment: Latest inc. player-compatible extension to monitors/RAII-lockers

This revision addresses some of Stan's requirements to align the Monitor/RAII-locker abstractions with the current Flash-runtime implementation.

Added:
- WaitNotifySafeMonitor (extends WaitNotifyMonitor to mirror the Flash-runtime's Monitor class)
- ScopedSafeLocker (extends ScopedLocker to mirror the Flash-runtime's Lock class)
- Slightly re-worked usage examples from Monitor.h

Comment 13

[Lars T Hansen]

If these were not in core but in VMPI or some other low-level directory (we really want to create a directory for code built on top of VMPI code that's shared between MMgc and the VM and the player, VMPI is not a good place for it - AvmAssert is one example of code recently moved into VMPI) we could get rid of the GCThreadLocal class, presumably.

(The logicistical problem here is the way the Player uses MMgc - it's compiled by itself before the VM, and it can be initialized without initializing the VM.  Thus MMgc should not depend on components in the VM, and preferably not on files in the VM's part of the directory tree.)

Comment 14

[Simon Wilkinson]

(In reply to comment #13)
> If these were not in core but in VMPI or some other low-level directory (we
> really want to create a directory for code built on top of VMPI code that's
> shared between MMgc and the VM and the player, VMPI is not a good place for it
> - AvmAssert is one example of code recently moved into VMPI) we could get rid
> of the GCThreadLocal class, presumably.

I've had problems with this for a while. Opened bug 591492

Comment 15

[Simon Wilkinson]

Attachment: Added AtomicOps class

Added atomic decrement and operator overloading to AtomicCounter32.

Added the AtomicOps class which packages many atomic operations:

class AtomicOps {
    public:
        static bool compareAndSwap32(int32_t oldValue, int32_t newValue, volatile int32_t* address);
        static bool compareAndSwap32WithBarrier(int32_t oldValue, int32_t newValue, volatile int32_t* address);
        static int32_t or32(uint32_t mask, volatile int32_t* address);
        static int32_t or32WithBarrier(uint32_t mask, volatile int32_t* address);
        static int32_t and32(uint32_t mask, volatile int32_t* address);
        static int32_t and32WithBarrier(uint32_t mask, volatile int32_t* address);
        static int32_t or32Prev(uint32_t mask, volatile int32_t* address);
        static int32_t or32WithBarrierPrev(uint32_t mask, volatile int32_t* address);
        static int32_t and32Prev(uint32_t mask, volatile int32_t* address);
        static int32_t and32WithBarrierPrev(uint32_t mask, volatile int32_t* address);
        static void memoryBarrier();
};

Comment 16

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

(In reply to comment #15)
> Created attachment 470062 [details] [diff] [review]
> Added AtomicOps class
> 
> 
> Added atomic decrement and operator overloading to AtomicCounter32.
> 
> Added the AtomicOps class which packages many atomic operations:
> 
> class AtomicOps {
>     public:
>         static bool compareAndSwap32(int32_t oldValue, int32_t newValue,
> volatile int32_t* address);
>         static bool compareAndSwap32WithBarrier(int32_t oldValue, int32_t
> newValue, volatile int32_t* address);
>         static int32_t or32(uint32_t mask, volatile int32_t* address);
>         static int32_t or32WithBarrier(uint32_t mask, volatile int32_t*
> address);
>         static int32_t and32(uint32_t mask, volatile int32_t* address);
>         static int32_t and32WithBarrier(uint32_t mask, volatile int32_t*
> address);
>         static int32_t or32Prev(uint32_t mask, volatile int32_t* address);
>         static int32_t or32WithBarrierPrev(uint32_t mask, volatile int32_t*
> address);
>         static int32_t and32Prev(uint32_t mask, volatile int32_t* address);
>         static int32_t and32WithBarrierPrev(uint32_t mask, volatile int32_t*
> address);
>         static void memoryBarrier();
> };

Shouldn't the above be a namespace, rather than a class?  Or am I missing something?

Comment 17

[Simon Wilkinson]

(In reply to comment #16)
> > class AtomicOps {
> >     public:
> >         static bool compareAndSwap32(int32_t oldValue, int32_t newValue,
> > volatile int32_t* address);
> >         static bool compareAndSwap32WithBarrier(int32_t oldValue, int32_t
> > newValue, volatile int32_t* address);
> >         static int32_t or32(uint32_t mask, volatile int32_t* address);
> >         static int32_t or32WithBarrier(uint32_t mask, volatile int32_t*
> > address);
> >         static int32_t and32(uint32_t mask, volatile int32_t* address);
> >         static int32_t and32WithBarrier(uint32_t mask, volatile int32_t*
> > address);
> >         static int32_t or32Prev(uint32_t mask, volatile int32_t* address);
> >         static int32_t or32WithBarrierPrev(uint32_t mask, volatile int32_t*
> > address);
> >         static int32_t and32Prev(uint32_t mask, volatile int32_t* address);
> >         static int32_t and32WithBarrierPrev(uint32_t mask, volatile int32_t*
> > address);
> >         static void memoryBarrier();
> > };
> 
> Shouldn't the above be a namespace, rather than a class?  Or am I missing
> something?

Referring to Lars' comment (13), all of the new concurrency stuff needs a proper place to live. So its arrangement in source dirs / namespaces / classes is up for discussion.

Comment 18

[Simon Wilkinson]

Attachment: Main patch: Rebased and cleaned-up

Comment 19

[Simon Wilkinson]

Attachment: Selftests

Comment 20

[Simon Wilkinson]

Attachment: Main patch. Latest.

Comment 21

[Simon Wilkinson]

Attachment: Selftests. Latest.

Comment 22

[Simon Wilkinson]

These latest patches are a stripped-down version of the concurrency library, with no extraneous (or contentious?) distractions.
Further patches will introduce all or some of the following:
OOM support, exception support, deadlock detection, safepoint support.

Talking points for this patch should include:

- The preferred style of block-scoped locking, with the MACRO sitting outside of the block (implemented as an if-statement that always evaluates to true). E.g.

SCOPE_LOCK(m_monitor) {
 // Critical section
}

- What should VMThread::currentThread() return for the main thread? Do we need to register primordial threads to give them some identity?

- Should we bother with start-up parameters for threads (e.g. guard size) if they can not be supported on all platforms?

Comment 23

[Stan Switzer]

One common failure mode of bare RAII is accidentally typing ScopedLocker<>(monitor) instead of ScopedLocker<>locker(monitor), which is, sadly, also valid but just means the wrong thing. So I guess the macros help prevent that.

I'm feeling an is-a relation rather than a has-a relation between WaitNotifyMonitor and RecursiveMutex, but it probably doesn't matter.

There are good uses for multiple condition variables, but that can be opened up easily enough when we need to cross that bridge.

I'm wondering whether a file name as generic as "Thread.h" is asking for include path problems down the road.  I searched my (Os X) computer and found several other files named Thread.h. None in any of our source trees/projects yet, but Boost has thread.hpp.  Maybe VMThread.h instead?

I'm being argued out of the PrivatizingMonitor concept. I think it has merit (we need all the declarative safety we can get when threading), but unless it can be used widely and consistently, then it is just localized "redecorating"  in various corners of the code. As much as it pain me to say it, I have to be down on redecorating.  So... it's good to see that it can be implemented in this model but I think we should we pull the plug on it. Sigh.

CyclicBarrier isn't quite like Java's and the UINT_MAX limitation is sorta scary.  Do we really need it?

There are gimmicks for getting a thread's name to show up in the debugger views on some platforms. Might be good to have a VMPI routine to set up that association.

It's rather annoying that priority can't be implemented correctly on POSIX, but hard to see what to do about it.

Something that's going to come up on Android, but can probably be handled in the VMPI layer is that a thread that is going to call into Java (and a lot will have to do that) will need to be created by Java. So the thread creation VMPI primitives will have to do that dance.

Comment 24

[Stan Switzer]

Something that might have slipped through the cracks: lock-acquisition-order deadlock prevention asserts.  It would be useful to get them incorporated at the lowest levels, I think.

Comment 25

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

(In reply to comment #24)
> Something that might have slipped through the cracks: lock-acquisition-order
> deadlock prevention asserts.  It would be useful to get them incorporated at
> the lowest levels, I think.

+1, assuming that our policy really *is* that deviations from acquisition order are bugs, are classified as such, and thus such asserts should never be considered to be "crying wolf."

[[ There was an e-mail thread amongst the runtime developers about this issue in a different context (Subject: "!probableDeadlock") over May 6-7, 2010. ]]

Since this is the sort of thing that only works if you get it in there before anyone writes policy-violating-but-deadlock-free code, we should try to get it in now if its going in at all.

Comment 26

[Simon Wilkinson]

(In reply to comment #24)
> Something that might have slipped through the cracks: lock-acquisition-order
> deadlock prevention asserts.  It would be useful to get them incorporated at
> the lowest levels, I think.

This is going in a later patch.

Refer to my last comment (22):
"These latest patches are a stripped-down version of the concurrency library,
with no extraneous (or contentious?) distractions.
Further patches will introduce all or some of the following:
OOM support, exception support, deadlock detection, safepoint support."

Comment 27

[Stan Switzer]

Yes, there should probably be a way to regster a VMThread object as the
currentThread() of a thread that was not created by this mechanism.  It should
probably be up to the invoker to create and destroy that thread object.
currentThread() might conceivably auto-allocate that but hard to see how it
would be freed. (Equiv of tls key-destructors hard (impossible?) to implement
in windows without a helper DLL).

Oh, by the way... as long as start() can fail, it might as well fail if the
thread is already started. You wouldn't want the same thread started twice.

(In reply to comment #22)
> - What should VMThread::currentThread() return for the main thread? Do we need
> to register primordial threads to give them some identity?

Comment 28

[Stan Switzer]

(In reply to comment #25)
> Since this is the sort of thing that only works if you get it in there before
> anyone writes policy-violating-but-deadlock-free code, we should try to get it
> in now if its going in at all.

You mean "policy-violating but 'deadlock-free' and will stay that way after the next unforeseen batch of changes."

Comment 29

[Stan Switzer]

The lazy/eager initialization of TLS could be a template parameter rather than a constructor parameter. Also, boolean parameters are really opaque for readers of code. It would almost be beter to have two templates with different names (because you don't need the isInitialized/initialize methods in the eager case). Or just decide on a single policy (hint: eager).  Lazy initialization (curiously, the default) seems to be asking for race conditions.

Comment 30

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

Comment on attachment 489260 [details] [diff] [review]
Main patch. Latest.

(am reviewing, but will not be done until tomorrow, hopefully early tomorrow.)

Comment 31

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

Comment on attachment 489260 [details] [diff] [review]
Main patch. Latest.

Four reasons for R-:

* I don't claim a long history with C++, so I am not biased towards a
  { RAII_LOCK(..); ... } style instead of RAII_LOCK(..) { ... } style
  proposed here (I would be a little surprised if people with such
  history preferred the latter).  But, consider the failure mode: if I
  type { RAII_LOCK(); ... } with your macros, its will be (1)
  confusing, (2) possibly overlooked, and (3) have bad consequences.
  So maybe better to just stick with the style that is used in other
  parts of the code base, e.g. with MMGC_LOCK.  (Then again, it looks
  like no one's complained about this, so maybe I am making a mountain
  out of a mole hill.)  Thoughts?  (Just convince me that you have
  buy-in from other developers, it won't take much to push me on this
  point!)

* You don't have any documentation for wait(), wait(int32_t),
  notify(), or notifyAll().  I can understand presuming knowledge on
  the part of your audience, but you should at least provide a link to
  reference documentation, so that we can all be on the same page.
  (I'm most familiar with Butenhof's pthreads book, but its
  terminology does not match the terms you use here, so I guess you
  can think of something better.)

  Also, if there are simple hints on usage, I recommend including
  them; e.g., you might say, adapting Butenhof's advice on signal
  versus broadcast, "Assuming correct waiter implementations, it is
  always safe to invoke notifyAll rather than notify, but not vice
  versa."

* In VMThread::start, what is a guard area size?

* In Thread.h, the literature references above the MemoryBarrier class
  definition seem very hardware-oriented (e.g. it doesn't matter for
  your clients that the SPARC v8 provides one memory model and the
  SPARC v9 another; what matters is the abstract memory model your
  library provides).

  1. At the very least, explicitly draw the connection from the
     barrier operation in the MemoryBarrier class to the equivalent
     operation in each paper.  (E.g., does MemoryBarrier::readWrite()
     act equivalently to Alpha's MB instruction described in Section
     6.4.3 of the Adve/Gharachorloo paper, and likewise to the smp_mb
     barrier in the Mckenney paper?).

  2. You might point the reader to specific subsection(s) of the
     Adve/Gharachorloo paper (e.g. section 6).

  3. If there's a good software-developer-oriented reference you can
     think of off hand, add it.  (I think of Butenhof as being an
     example of a software-developer-oriented reference, but again its
     terminology does not exactly match yours, so there's an impedence
     mismatch there.)

Other feedback:

nit: In example code in comments above the ScopedLocker, you are have
Foo::foo qualification on methods within class Foo { ... }.  Its not a
big deal but you might as well take the methods out of the class Foo {
... } context to save on wasted indentation.

typo above AtomicCounter32 in Thread.h: "deccAndGet"

Should the AtomicCounter32::set method be renamed to declare in its
name that it is barrier-less?

Above the VMThreadLocal: "The type T must cast to void*", three points:

1. At the very least, this should say "The type T must
   reinterpret_cast to and from void*".  (I at first interpreted the
   original text as saying that it would suffice for T to implement
   operator void*(), but that is false on two counts.)
2. Also, would a STATIC_ASSERT(sizeof(T) == sizeof(void*)) be
   reasonable and desirable?
3. Is a reinterpret_cast actually the right thing to use when casting
   to/from void*?  It looks to me like it is planned for C++0x, but
   not currently legal [1].  I guess the more important criteria is:
   what does current codebase do?  (A casual grep makes it appear that
   we usually use C-style casts when a void* is involved.)
   [1] http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1120

In the enum VMThread::ThreadState, add a note that the enum ordering
significant (since you are using < TERMINATED in join).  Actually,
why not just use != TERMINATED?

Cut-and-paste error in doc above VMThread(Runnable& runnable); you
reference a non-existent @param name there.

Nit: I would prefer the phrase "makes own copy of the string" to the
passive parenthetical you have.

The documentation above VMThread's currentThread() method implies that
it is legal to invoke currentThread() from contexts not created by
start(); it just isn't legal to rely on the value returned.  Can we
weaken its guarantees, and just say that it is illegal to call it from
contexts not created by start()?  Or is there some context where you
expect to want to invoke currentThread() without a useful return
value?  (The comments on the ticket indicate that you are thinking we
should instead go in the other direction, and extend the API to allow
registration of primordial threads, so I'll cut this question short.)

In VMThread::VMThread constructors, please put a line comment
describing which warning you're disabling via the pragma there.

In the code for VMThread::startInternal, why use SCOPE_LOCK_NAMED for
the first block?  You don't use the bound locker variable in the
block.

Comment 32

[Krzysztof Palacz]

> * I don't claim a long history with C++, so I am not biased towards a
>   { RAII_LOCK(..); ... } style instead of RAII_LOCK(..) { ... } style
>   proposed here (I would be a little surprised if people with such
>   history preferred the latter).  But, consider the failure mode: if I
>   type { RAII_LOCK(); ... } with your macros, its will be (1)
>   confusing, (2) possibly overlooked, and (3) have bad consequences.
>   So maybe better to just stick with the style that is used in other
>   parts of the code base, e.g. with MMGC_LOCK.  (Then again, it looks
>   like no one's complained about this, so maybe I am making a mountain
>   out of a mole hill.)  Thoughts?  (Just convince me that you have
>   buy-in from other developers, it won't take much to push me on this
>   point!)
FWIW, I'm fine with the synchronized (monitor)  { ... } style. 
Maybe the bool() operator could record in debug mode that it's been called, and wait(),notify() and ~ScopedLocker() would assert later if it hadn't? It's not perfect but it should enforce the style.

Comment 33

[Tommy Reilly]

For what its worth I like the LOCK(..) { } style better and would support converting MMGC_ENTER and friends over to it (no I'm not volunteering).

Comment 34

[Simon Wilkinson]

I prefer the LOCK(monitor) {...} style too, but my two misgivings are:

- As Felix says, if you're very used do writing {LOCK(monitor); ...}, then you could think you have the lock, but in fact you don't. I don't think Krzysztof's suggested asserts wouldn't actually help much here, as it's the acquire we really care about (BTW, the ~ScopedLocker() assert would never fire, wait() already asserts lock ownership, and notify() should do too -- I'll add this in the revised patch). Having said this, if your locking protocol becomes anything other than trivial, then you should really be explicitly asserting lock ownership when not in the lexical scope of the RAII locker.

- Because of the if-block trick, compilers can spit 'missing return in non-void function' warnings if your SCOPE_LOCK block sits at the end of a function and you return from within the block. For example:

int foo() {
    SCOPE_LOCK(m_monitor) {
        return m_value;
    }
    // Although this is unreachable code (the if should be optimized away) the compiler can still warn about a missing return.
}

I've not found this to be a problem so far...

Comment 35

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

(In reply to comment #34) 
> - Because of the if-block trick, compilers can spit 'missing return in non-void
> function' warnings if your SCOPE_LOCK block sits at the end of a function and
> you return from within the block.

Another nightmare hypothetical scenario:

if (TEST)
  SCOPE_LOCK(m) { ... }
else
  somethingelse; 

Fun fun fun. :(

But so far you actually have a fair amount of buy-in from the VM team.  Do we need buy-in from the player side, or will this stay within the VM?

Comment 36

[Stan Switzer]

(In reply to comment #35)

> if (TEST)
>   SCOPE_LOCK(m) { ... }
> else
>   somethingelse; 
> 
> Fun fun fun. :(

Gasp! But it is fixable, no?

 #define SCOPE_LOCK_NAMED(_name_, _m_)     if (vmbase::ScopedLocker<> _name_ = _m_) {} else

and then reverse the sense of the bool conversion. Ugh either way, I guess.

Comment 37

[Krzysztof Palacz]

> - As Felix says, if you're very used do writing {LOCK(monitor); ...}, then you
> could think you have the lock, but in fact you don't. I don't think Krzysztof's
> suggested asserts wouldn't actually help much here, as it's the acquire we
> really care about (BTW, the ~ScopedLocker() assert would never fire, wait()
> already asserts lock ownership, and notify() should do too -- I'll add this in
> the revised patch). 
Ah yes, that's true, the assert would discourage this style: { ScopedLocker l(m); ... } but the macro could still be misused.

Comment 38

[Steven Johnson]

(In reply to comment #35)
> if (TEST)
>   SCOPE_LOCK(m) { ... }
> else
>   somethingelse; 

ahem, non-braced if statement should be rejected by code review in any case...

Comment 39

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

(In reply to comment #38)
> (In reply to comment #35)
> > if (TEST)
> >   SCOPE_LOCK(m) { ... }
> > else
> >   somethingelse; 
> 
> ahem, non-braced if statement should be rejected by code review in any case...

Tamarin Coding Standards don't mandate always including braces.  I personally prefer them, but some developers on the team eschew so-called "extra" braces.

And did you ever get that pony  (Bug 609294, comment 13)?  :)

Comment 40

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

(In reply to comment #36)
> (In reply to comment #35)
> 
> > if (TEST)
> >   SCOPE_LOCK(m) { ... }
> > else
> >   somethingelse; 
> > 
> > Fun fun fun. :(
> 
> Gasp! But it is fixable, no?
> 
>  #define SCOPE_LOCK_NAMED(_name_, _m_)     if (vmbase::ScopedLocker<> _name_ =
> _m_) {} else
> 
> and then reverse the sense of the bool conversion. Ugh either way, I guess.

Nice work!  Okay, so I'd propose that Simon adopt this variant, assuming he does get buy-in for this format.

Comment 41

[Simon Wilkinson]

(In reply to comment #35)
> Another nightmare hypothetical scenario:
> 
> if (TEST)
>   SCOPE_LOCK(m) { ... }
> else
>   somethingelse; 
> 
> Fun fun fun. :(

Would this not be caught by -Wparentheses?

Although, as Stan stated, ugh.

Hmmmmm. For me, the stylistic benefits are still winning out...

Comment 42

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

Comment on attachment 489261 [details] [diff] [review]
Selftests. Latest.

big nit: please remove the cut-and-pasto that refers to Bugzilla 543560.

Comment 43

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

Comment on attachment 489261 [details] [diff] [review]
Selftests. Latest.

R+; comments below:

When you remove the cut-and-pasto, replace it with a short note at the top indicating what the big picture strategy is for most of the tests.  Here's a sample to get the idea across of what I'm thinking of: "Each construct is tested by (1) using it in the implementation of a mutator that modifies a counter for a fixed number of iterations, and then (2) running duplicates of that mutator in parallel.  The final counter value ends up in sharedCounter, which is guarded by m_monitor (except for in CASTest).  Each test checks that the sharedCounter ends up with a statically determined end value."  [Don't use that text without sanity checking it, of course.]

For the MemoryBarrierTest, are all of those barriers necessary?  My impression is that many are not.  Since the goal of testing is to expose bugs, I argue that you should insert some minimal set of barriers that would still yield a correct Dekker lock if the underlying barrier implementation is correct.
Here is one sample article with an analysis of Dekker that includes explicit fence instructions, so you could use that as the basis for deciding where your barriers should go.
http://www.justsoftwaresolutions.co.uk/threading/implementing_dekkers_algorithm_with_fences.html

For the CASTest, is the with/without barrier selection actually making any difference in the test?  As far as I can tell the two bodies are identical, which means this could not detect a compareAndSwap32WithBarrier that was actually missing the barrier.  I don't have an immediate suggestion for how to actually test the presence of the barrier in other version, but include a note saying that it is missing so that people don't spend time hunting for it.

Comment 44

[Krzysztof Palacz]

Comment on attachment 489260 [details] [diff] [review]
Main patch. Latest.

Couple of minor comments:
* What is the right protocol to free VMThread objects? Judging from the implementation, only after a join(), which also means that threads cannot clean up after themselves, and there has to be at least one thread willing to block on join() and clean up. If this is the case, it should be documented (it's also kindof inconvenient).

* ~VMThread() accesses m_state outside of a critical section, it seems correct to me but a comment could be useful. Actually, would it ever be OK to call ~VMThread() when m_state is not TERMINATED?

* Initialization of VMThreadLocal appears to not be thread safe, should that be documented as well?

* It would be nice if in debug mode the SCOPE_LOCK macro detected when the lock is not being released because of an exception. I know it's been debated but it would have saved me some debugging time already.

* In the comments in Thread.h:179
   ScopedLocker(m_monitor) locker; // Lock m_monitor
should that be 
   ScopedLocker<> locker(m_monitor);

* Thread-inlines.h:70 line ends in two semicolons (or is there a secret purpose to this?)

Comment 45

[Simon Wilkinson]

Attachment: Main patch with review fixes.

Stan's comments:

> One common failure mode of bare RAII is accidentally typing
> ScopedLocker<>(monitor) instead of ScopedLocker<>locker(monitor), which is,
> sadly, also valid but just means the wrong thing. So I guess the macros help
> prevent that.

Now that we aren't going to bother with the privatizing versions of the monitors there's no immediate need for the ScopedLockers to be templatized. They're de-templatized in the latest patch.

> I'm feeling an is-a relation rather than a has-a relation between
> WaitNotifyMonitor and RecursiveMutex, but it probably doesn't matter.

Ok, I agree. In the new patch:
class WaitNotifyMonitor : public RecursiveMutex

Also, I've added a new scoped-locker for RecursiveMutexes. So now we have:
MutexLocker(RecursiveMutex& monitor)
and 
MonitorLocker(WaitNotifyMonitor& monitor)

> There are good uses for multiple condition variables, but that can be opened up
> easily enough when we need to cross that bridge.

Indeed. I've not needed multiple conditions in any of the feature work that uses this library. But I'm sure I will at some point.

> I'm wondering whether a file name as generic as "Thread.h" is asking for
> include path problems down the road.  I searched my (Os X) computer and found
> several other files named Thread.h. None in any of our source trees/projects
> yet, but Boost has thread.hpp.  Maybe VMThread.h instead?

Good point. In the patch I've renamed to VMThread.h.

> I'm being argued out of the PrivatizingMonitor concept. I think it has merit
> (we need all the declarative safety we can get when threading), but unless it
> can be used widely and consistently, then it is just localized "redecorating" 
> in various corners of the code. As much as it pain me to say it, I have to be
> down on redecorating.  So... it's good to see that it can be implemented in
> this model but I think we should we pull the plug on it. Sigh.

Removed in the new patch.

> CyclicBarrier isn't quite like Java's and the UINT_MAX limitation is sorta
> scary.  Do we really need it?

Java gets limitless barrier generations for free by using an object rather than a counter, and letting the GC do the work. I don't actually use the barrier in any of the later patches, so let's just remove it for now.

> There are gimmicks for getting a thread's name to show up in the debugger views
> on some platforms. Might be good to have a VMPI routine to set up that
> association.

It looks simple for Windows and OSX 10.6.
Logged in bug 613916
	
> Something that's going to come up on Android, but can probably be handled in
> the VMPI layer is that a thread that is going to call into Java (and a lot will
> have to do that) will need to be created by Java. So the thread creation VMPI
> primitives will have to do that dance.

Ok, we'll handle that one when we get to it.

> Something that might have slipped through the cracks: lock-acquisition-order
> deadlock prevention asserts.  It would be useful to get them incorporated at
> the lowest levels, I think.

This is coming in a later patch.
See bug 613920

> Yes, there should probably be a way to regster a VMThread object as the
> currentThread() of a thread that was not created by this mechanism.  It should
> probably be up to the invoker to create and destroy that thread object.
> currentThread() might conceivably auto-allocate that but hard to see how it
> would be freed. (Equiv of tls key-destructors hard (impossible?) to implement
> in windows without a helper DLL).

Logged as bug 614180

> Oh, by the way... as long as start() can fail, it might as well fail if the
> thread is already started. You wouldn't want the same thread started twice.

Implemented in the revised patch.

> The lazy/eager initialization of TLS could be a template parameter rather than
> a constructor parameter. Also, boolean parameters are really opaque for readers
> of code. It would almost be beter to have two templates with different names
> (because you don't need the isInitialized/initialize methods in the eager
> case). Or just decide on a single policy (hint: eager).  Lazy initialization
> (curiously, the default) seems to be asking for race conditions.

I've removed the lazy initialization altogether. It wasn't needed.


Felix's comments:

 
> * I don't claim a long history with C++, so I am not biased towards a
>   { RAII_LOCK(..); ... } style instead of RAII_LOCK(..) { ... } style
>   proposed here (I would be a little surprised if people with such
>   history preferred the latter).  But, consider the failure mode: if I
>   type { RAII_LOCK(); ... } with your macros, its will be (1)
>   confusing, (2) possibly overlooked, and (3) have bad consequences.
>   So maybe better to just stick with the style that is used in other
>   parts of the code base, e.g. with MMGC_LOCK.  (Then again, it looks
>   like no one's complained about this, so maybe I am making a mountain
>   out of a mole hill.)  Thoughts?  (Just convince me that you have
>   buy-in from other developers, it won't take much to push me on this
>   point!)

I'd class the discussions above as buy-in, so I'll leave it in for now. I'm in no way precious about keeping this style though.


> * You don't have any documentation for wait(), wait(int32_t),
>   notify(), or notifyAll().  I can understand presuming knowledge on
>   the part of your audience, but you should at least provide a link to
>   reference documentation, so that we can all be on the same page.
>   (I'm most familiar with Butenhof's pthreads book, but its
>   terminology does not match the terms you use here, so I guess you
>   can think of something better.)
> 
>   Also, if there are simple hints on usage, I recommend including
>   them; e.g., you might say, adapting Butenhof's advice on signal
>   versus broadcast, "Assuming correct waiter implementations, it is
>   always safe to invoke notifyAll rather than notify, but not vice
>   versa."

I've added lots more docs in the new patch. Particularly WaitNotifyMonitor.

> * In VMThread::start, what is a guard area size?

Extra memory beyond the stack pointer that buffers/signals stack overflow.

> * In Thread.h, the literature references above the MemoryBarrier class
>   definition seem very hardware-oriented (e.g. it doesn't matter for
>   your clients that the SPARC v8 provides one memory model and the
>   SPARC v9 another; what matters is the abstract memory model your
>   library provides).
> 
>   1. At the very least, explicitly draw the connection from the
>      barrier operation in the MemoryBarrier class to the equivalent
>      operation in each paper.  (E.g., does MemoryBarrier::readWrite()
>      act equivalently to Alpha's MB instruction described in Section
>      6.4.3 of the Adve/Gharachorloo paper, and likewise to the smp_mb
>      barrier in the Mckenney paper?).
> 
>   2. You might point the reader to specific subsection(s) of the
>      Adve/Gharachorloo paper (e.g. section 6).
> 
>   3. If there's a good software-developer-oriented reference you can
>      think of off hand, add it.  (I think of Butenhof as being an
>      example of a software-developer-oriented reference, but again its
>      terminology does not exactly match yours, so there's an impedence
>      mismatch there.)

Agreed, this was quite lazy documentation. I've documented the MemoryBarrier::readWrite() function more precisely. I've added a FIXME to provide programmer-oriented references.

> Should the AtomicCounter32::set method be renamed to declare in its
> name that it is barrier-less?

I think it should actually have the barrier to make it uniform across all the class's methods. Fixed in the new patch.

> Above the VMThreadLocal: "The type T must cast to void*", three points:
> 
> 1. At the very least, this should say "The type T must
>    reinterpret_cast to and from void*".  (I at first interpreted the
>    original text as saying that it would suffice for T to implement
>    operator void*(), but that is false on two counts.)

Fixed.

> 2. Also, would a STATIC_ASSERT(sizeof(T) == sizeof(void*)) be
>    reasonable and desirable?

Yes. Although we currently cannot get to static_assert from vmbase.
Raised bug 614135.

> 3. Is a reinterpret_cast actually the right thing to use when casting
>    to/from void*?  It looks to me like it is planned for C++0x, but
>    not currently legal [1].  I guess the more important criteria is:
>    what does current codebase do?  (A casual grep makes it appear that
>    we usually use C-style casts when a void* is involved.)
>    [1] http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1120

It looks like reinterpret_cast is not the correct cast.
FYI, Bjarne Stroustrup says to use a static_cast or a c-style cast: http://www2.research.att.com/~bs/bs_faq2.html#void-ptr

The static_cast isn't going to work with uintptr_t, so it's going to have to be the c-style cast.

> In the enum VMThread::ThreadState, add a note that the enum ordering
> significant (since you are using < TERMINATED in join).  Actually,
> why not just use != TERMINATED?

I orginally had some more states, so the < is left over from that.
Removed in the latest patch.

> The documentation above VMThread's currentThread() method implies that
> it is legal to invoke currentThread() from contexts not created by
> start(); it just isn't legal to rely on the value returned.  Can we
> weaken its guarantees, and just say that it is illegal to call it from
> contexts not created by start()?  Or is there some context where you
> expect to want to invoke currentThread() without a useful return
> value?  (The comments on the ticket indicate that you are thinking we
> should instead go in the other direction, and extend the API to allow
> registration of primordial threads, so I'll cut this question short.)

Until primordial threads can acquire VMThread identity (see bug 614180) I think it's best that currentThread() return NULL as a valid value for this case. I've updated the docs.

> In VMThread::VMThread constructors, please put a line comment
> describing which warning you're disabling via the pragma there.

Better still: I got rid of the warning.

> In the code for VMThread::startInternal, why use SCOPE_LOCK_NAMED for
> the first block?  You don't use the bound locker variable in the
> block.

Historically it was due to name mangling issue done by the macro. The point is now moot however, so I've changed it to SCOPE_LOCK.

(In reply to comment #40)
> (In reply to comment #36)
> > (In reply to comment #35)
> > 
> > > if (TEST)
> > >   SCOPE_LOCK(m) { ... }
> > > else
> > >   somethingelse; 
> > > 
> > > Fun fun fun. :(
> > 
> > Gasp! But it is fixable, no?
> > 
> >  #define SCOPE_LOCK_NAMED(_name_, _m_)     if (vmbase::ScopedLocker<> _name_ =
> > _m_) {} else
> > 
> > and then reverse the sense of the bool conversion. Ugh either way, I guess.
> 
> Nice work!  Okay, so I'd propose that Simon adopt this variant, assuming he
> does get buy-in for this format.

The macro now uses this variant, and the RAII lockers have been changed to use:
operator bool () const {return false;}

Krzysztof's Commments


> * What is the right protocol to free VMThread objects? Judging from the
> implementation, only after a join(), which also means that threads cannot clean
> up after themselves, and there has to be at least one thread willing to block
> on join() and clean up. If this is the case, it should be documented (it's also
> kindof inconvenient).

At the moment I don't see how a thread can free its own VMThread as it will not know how it was allocated, or if other threads still reference it (unless it is explicitly told these things and can assume that they will remain true until it needs to performs the free). So it's really a question of how do we know when nothing references the VMThread. Well that's up to the freeing thread to know. One thing the freeing thread has to find out is if the VMThread's native thread has exited (and so has notified all joiners) and if all the joiners have released the VMThread's m_joinMonitor after being notified. So in fact, having the freeing thread just join the VMThread isn't actually enough, the dtor must explicitly wait for the joiners to release the mutex. To perform this we cannot use the thread's m_status, as the freeing thread may see the update to TERMINATED before all the joiners have done. So we have to count joining threads in and out, and have the VMThread dtor wait until there are zero joiners.

Did you have something else in mind?

I've documented this in the code and also added a new self test.
Also note the next comment about VMPI_threadDetach.

> * ~VMThread() accesses m_state outside of a critical section, it seems correct
> to me but a comment could be useful. Actually, would it ever be OK to call
> ~VMThread() when m_state is not TERMINATED?

In the new patch VMPI_threadDetach is performed safely by the exiting native thread, rather than in ~VMThread(). So the access of m_state is not needed.

> * Initialization of VMThreadLocal appears to not be thread safe, should that be
> documented as well?

The lazy init has been removed in the latest patch, so it's thread-safe.

> * It would be nice if in debug mode the SCOPE_LOCK macro detected when the lock
> is not being released because of an exception. I know it's been debated but it
> would have saved me some debugging time already.

I've had this in the patch previously and then removed it.
You can see it here:
https://bug555765.bugzilla.mozilla.org/attachment.cgi?id=457235
The macro was SCOPE_LOCK_RETHROW. It caught AS exceptions, unlocked the monitor, and then rethrew. So it can be tweaked to assert instead.
Re-introduction of exception support is coming in a later bug.

> * In the comments in Thread.h:179
>    ScopedLocker(m_monitor) locker; // Lock m_monitor
> should that be 
>    ScopedLocker<> locker(m_monitor);

Fixed.

> * Thread-inlines.h:70 line ends in two semicolons (or is there a secret purpose
> to this?)

It means 'I really, really want this statement to end'.

Comment 46

[Simon Wilkinson]

Attachment: Selftests with review fixes

(In reply to comment #43)
> Comment on attachment 489261 [details] [diff] [review]
> Selftests. Latest.
> 
> R+; comments below:
> 
> When you remove the cut-and-pasto, replace it with a short note at the top
> indicating what the big picture strategy is for most of the tests.  Here's a
> sample to get the idea across of what I'm thinking of: "Each construct is
> tested by (1) using it in the implementation of a mutator that modifies a
> counter for a fixed number of iterations, and then (2) running duplicates of
> that mutator in parallel.  The final counter value ends up in sharedCounter,
> which is guarded by m_monitor (except for in CASTest).  Each test checks that
> the sharedCounter ends up with a statically determined end value."  [Don't use
> that text without sanity checking it, of course.]


Comment added and attributed.


> For the MemoryBarrierTest, are all of those barriers necessary?  My impression
> is that many are not.  Since the goal of testing is to expose bugs, I argue
> that you should insert some minimal set of barriers that would still yield a
> correct Dekker lock if the underlying barrier implementation is correct.
> Here is one sample article with an analysis of Dekker that includes explicit
> fence instructions, so you could use that as the basis for deciding where your
> barriers should go.
> http://www.justsoftwaresolutions.co.uk/threading/implementing_dekkers_algorithm_with_fences.html


No they aren't all necessary. It's an extremely conservative implementation. My argument was that it would be a better smoke-test if a conservative implementation failed rather than optimized one. I would spend more time checking the optimized implementation for bugs than the barrier implementations. Note that the Dekker lock is identical to that I used in the VMPI threading self-tests. From this I have copied the docs and added a reference to the existing bug 609943, which is intended to track memory-barrier testing strategies.


> For the CASTest, is the with/without barrier selection actually making any
> difference in the test?  As far as I can tell the two bodies are identical,
> which means this could not detect a compareAndSwap32WithBarrier that was
> actually missing the barrier.  I don't have an immediate suggestion for how to
> actually test the presence of the barrier in other version, but include a note
> saying that it is missing so that people don't spend time hunting for it.


I don't really understand what you mean. One loop body calls the barrier version, the other, the barrier-less version. So far I don't have a strategy for testing the presence of the barrier, just that the CAS works as described.

Comment 47

[Simon Wilkinson]

I just noticed that MutexLocker declares this:

const RecursiveMutex& operator=(const RecursiveMutex& locker);

instead of this:

const MutexLocker& operator=(const MutexLocker& locker);

I'll change this before it lands, but not in the current patches (reduce Bugzilla traffic)

Comment 48

[Krzysztof Palacz]

Comment on attachment 492602 [details] [diff] [review]
Main patch with review fixes.

There's a FIXME about background reading on membars.

Otherwise, some very minor nits, feel free to ignore.

isLockedByCurrentThread(): wouldn't it be nice if it were available in non-debug builds? It currently makes sense for it to be debug-only, but one could imagine a layered framework that would want to expose such information as well, in which case it would have to replicate the debug-only functionality (in particular, if we had AS-level monitors, the AS monitor might want to expose isLockedByCurrentThreads()

I think I'd prefer the assert in VMThread::join() to be a dynamic check. I guess VMThread::start() should always be tested for success, but the code would be more resilient in case the test is forgotten and, e.g.,  thread creation randomly fails.

It's documented so I guess it's fine, but wait() in destructors sort of rubs me the wrong way. An explicit dispose() and an assert in ~VMThread would be one alternative.

Comment 49

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

Comment on attachment 492602 [details] [diff] [review]
Main patch with review fixes.

R+.

The new docs look good.  If I am reminded of a good
programmer-oriented reference soon, I'll let you know.

uber consistency nit in vmbase/VMThread.h: above one
start() you write "As of Nov' 2010" and above another
you write "As of Nov 2010"

Comment 50

[Simon Wilkinson]

Attachment: Latest patch. Added REALLY_INLINE to get/set of VMThreadLocal

A small addition:
Added REALLY_INLINE to get/set of VMThreadLocal

Comment 51

[Edwin Smith]

Only nit:  open brace on separate lines for toplevel functions.  if you see breaks from this style, point them out or fix.

Braces in functions inside classes are where we've tended to have style drift, but at the top level of source files they should be on separate lines.

Comment 52

[Simon Wilkinson]

Attachment: Selftests latest

Fixed local object declaration in vmbase_concurrency selftest

Comment 53

[Simon Wilkinson]

Attachment: Main patch latest

Changes:
- Rebased
- Fixed the Visual C++ build
- Fixed race condition between thread detach and VMThread free.
- Updated memory barrier docs
- Formatted to the house coding style
- Declared VMThread::m_state to be non-volatile (was required historically)
- Added missing static declaration to recordStackBoundariesAndSleep()

Comment 54

[Tamarin Bot]

changeset: 5706:cffa7060d9bc
user:      [e-mail]
summary:   Bug 555765 - Cross-platform C++ concurrency abstractions built from VMPI extensions (p=siwilki,r+kpalacz,r+fklockii,sr+edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/cffa7060d9bc

Comment 55

[Tamarin Bot]

changeset: 5707:d8b78be5b40f
user:      [e-mail]
summary:   Selftests for Bug 555765 - Cross-platform C++ concurrency abstractions built from VMPI extensions (p=siwilkir+fklockii,sr+edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/d8b78be5b40f

Comment 56

[Brent Baker]

Comment on attachment 499299 [details] [diff] [review]
Selftests latest

Can we please have this selftest removed from the smokes list? There is no need for this to be in the smokes list since it is NOT a known failure and since it is in the list it is being run on every machine during the smokes phase which is meant to try and catch recent failures:

http://hg.mozilla.org/tamarin-redux/diff/d8b78be5b40f/test/runsmokes.txt

Comment 57

[Simon Wilkinson]

(In reply to comment #56)
> Comment on attachment 499299 [details] [diff] [review]
> Selftests latest
> 
> Can we please have this selftest removed from the smokes list? There is no need
> for this to be in the smokes list since it is NOT a known failure and since it
> is in the list it is being run on every machine during the smokes phase which
> is meant to try and catch recent failures:
> 
> http://hg.mozilla.org/tamarin-redux/diff/d8b78be5b40f/test/runsmokes.txt

Will do. (BTW, bug 555760 is doing something very similar).
This leaves me with some testing process questions though. I'll catch you on IRC.