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

RESOLVED FIXED

Status

Tamarin
Virtual Machine
--
enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Adam Welc, Assigned: Adam Welc)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
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
(Assignee)

Comment 1

7 years ago
Created attachment 520034 [details] [diff] [review]
Modifies the multi-worker implementation to use the cross-platform threading API

Tested on Mac OS X and on Windows 7
Assignee: nobody → awelc
Attachment #520034 - Flags: superreview?(lhansen)
Attachment #520034 - Flags: review?(siwilkin)
Attachment #520034 - Flags: feedback?(kpalacz)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
Created attachment 520068 [details] [diff] [review]
Modifies the multi-worker implementation to use the cross-platform threading API

Corrects synchronization construct naming in the comments
Attachment #520034 - Attachment is obsolete: true
Attachment #520034 - Flags: superreview?(lhansen)
Attachment #520034 - Flags: review?(siwilkin)
Attachment #520034 - Flags: feedback?(kpalacz)
Attachment #520068 - Flags: superreview?(lhansen)
Attachment #520068 - Flags: review?(siwilkin)
Attachment #520068 - Flags: feedback?(kpalacz)

Comment 4

7 years ago
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.
Attachment #520068 - Flags: superreview?(lhansen)
(Assignee)

Updated

7 years ago
Attachment #520068 - Flags: feedback?(kpalacz) → superreview?(kpalacz)

Comment 5

7 years ago
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?
Attachment #520068 - Flags: review?(siwilkin) → review+

Comment 6

7 years ago
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.
Attachment #520068 - Flags: superreview?(kpalacz) → superreview+
(Assignee)

Comment 7

7 years ago
Created attachment 523444 [details] [diff] [review]
Modifies the multi-worker implementation to use the cross-platform threading API

Fixed nits noted in Simon's most recent review
(Assignee)

Updated

7 years ago
Attachment #520068 - Attachment is obsolete: true

Comment 8

7 years ago
changeset: 6173:1cdf89b63cf1
user:      kpalacz@adobe.com
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

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.