642609 - Multi-worker implementation does not use cross-platform threading API (relies on pthreads instead)

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Adam Welc]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-US) AppleWebKit/534.7 (KHTML, like Gecko) Chrome/7.0.517.41 Safari/534.7
Build Identifier: 

Shell-only problem.

Reproducible: Always

Comment 1

[Adam Welc]

Attachment: Modifies the multi-worker implementation to use the cross-platform threading API

Tested on Mac OS X and on Windows 7

Comment 2

[Simon Wilkinson]

Comment on attachment 520034 [details] [diff] [review]
Modifies the multi-worker implementation to use the cross-platform threading API


Quick first pass:
The docs still refer to the old pthread mutexes and condition variables called 'm' and 'c' respectively.

Comment 3

[Adam Welc]

Attachment: Modifies the multi-worker implementation to use the cross-platform threading API

Corrects synchronization construct naming in the comments

Comment 4

[Lars T Hansen]

Comment on attachment 520068 [details] [diff] [review]
Modifies the multi-worker implementation to use the cross-platform threading API

Removing self from SR, suggest you get SR from Krzysztof instead.

Comment 5

[Simon Wilkinson]

Comment on attachment 520068 [details] [diff] [review]
Modifies the multi-worker implementation to use the cross-platform threading API

Nits:

avmshell.cpp/96:
typo: heigh

avmshell.cpp/99:
You say you're going to use VMPI_threadAttrDefaultStackSize() when creating the threads, but you don't. Do you mean that calling VMThread::start() without specifying a stack size will use the default value? In fact, maybe the old comment about assuming being called early is more useful than your new comment.

static void masterThread(MultiworkerState& state):
The 3 nested 'if (finish) break;' statements, can't those tests just go in the loop-headers?

Comment 6

[Krzysztof Palacz]

Comment on attachment 520068 [details] [diff] [review]
Modifies the multi-worker implementation to use the cross-platform threading API

Simon's comments are worth addressing. I can land once you post an updated patch.

Comment 7

[Adam Welc]

Attachment: Modifies the multi-worker implementation to use the cross-platform threading API

Fixed nits noted in Simon's most recent review

Comment 8

[Tamarin Bot]

changeset: 6173:1cdf89b63cf1
user:      [e-mail]
summary:   Bug 642609 - Multi-worker implementation does not use cross-platform threading API (relies on pthreads instead) (p=awelc,r+swilkin,sr+kpalacz)

http://hg.mozilla.org/tamarin-redux/rev/1cdf89b63cf1