If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

VMPI threading extensions require a yield-like function

RESOLVED FIXED

Status

Tamarin
Virtual Machine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Simon Wilkinson, Unassigned)

Tracking

(Blocks: 1 bug)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
A non-priority-inverting yield will be very useful for building well-behaved busy-waiting.
(Reporter)

Updated

7 years ago
Blocks: 575544
(Reporter)

Updated

7 years ago
Blocks: 562687
(Reporter)

Comment 1

7 years ago
Created attachment 489741 [details] [diff] [review]
Initial Patch
(Reporter)

Comment 2

7 years ago
Created attachment 494807 [details] [diff] [review]
Latest patch
Attachment #489741 - Attachment is obsolete: true
Attachment #494807 - Flags: review?(kpalacz)
Attachment #494807 - Flags: feedback?(stan)

Updated

7 years ago
Attachment #494807 - Flags: review?(kpalacz) → review+
(Reporter)

Updated

7 years ago
Attachment #494807 - Flags: superreview?(edwsmith)

Comment 3

7 years ago
Comment on attachment 494807 [details] [diff] [review]
Latest patch

Looking at this as a newcomer, I read the docs on VMPI_yeild(), and wonder what eactly it's good for, given the contract on different platforms is so loose.  

* are there cases where it MUST be used? what are they?
* what is guaranteed to happen, even if implementations vary so much?
Attachment #494807 - Flags: superreview?(edwsmith) → superreview+
(Reporter)

Comment 4

7 years ago
(In reply to comment #3)
> Comment on attachment 494807 [details] [diff] [review]
> Latest patch
> 
> Looking at this as a newcomer, I read the docs on VMPI_yeild(), and wonder what
> eactly it's good for, given the contract on different platforms is so loose.  
> 
> * are there cases where it MUST be used? what are they?
> * what is guaranteed to happen, even if implementations vary so much?

A yield is useful within busy-waiting loops.
However, there is no requirement that it *must* be used, except on platforms without a pre-emptive thread scheduler. I understand that all of our platforms do have a pre-emptive scheduler, so its application will only be an optimization.

I'd really like the yield function's contract to be:
Hints to the scheduler that the running thread is waiting on some condition to be updated by another thread, and importantly, that the condition is already fulfilled or will be very soon (i.e. within a scheduling quantum). Hence the running thread should temporarily get out of the way (yield) for the other thread to run. This should not penalize the regular scheduling of either thread, and particularly we want the yielding thread to start running again as soon as possible as it can now make forward progress.

On Windows the SwitchToThread function is close enough to the above idea. All the other platforms aren't particularly clear about when the yielding thread will start running again (getting put to the back of runnable queue is typical).

In most cases you should be using regular wait/notify to synchronize threads in this way. But in some parts of the synchronization implementation itself you cannot, i.e. spin-lock implementation, safepoint wait etc. So the yield is better than nothing.
(Reporter)

Comment 5

7 years ago
Created attachment 497757 [details] [diff] [review]
Latest. More docs.


Added some more docs that summarizes my reply to Ed above.
Attachment #494807 - Attachment is obsolete: true
Attachment #494807 - Flags: feedback?(stan)
(Reporter)

Comment 6

7 years ago
Created attachment 499307 [details] [diff] [review]
Latest with Solaris and WinCE build fixes

Fixes the WinCE build where SwitchToThread() is not available (Sleep(1) is used instead).

Fixes the Solaris build so that the rt library is linked (required for the sched_yield() function)
Attachment #497757 - Attachment is obsolete: true
(Reporter)

Comment 7

7 years ago
Created attachment 499317 [details] [diff] [review]
Latest. Rebased to TR-5705
Attachment #499307 - Attachment is obsolete: true
(Reporter)

Comment 8

7 years ago
Comment on attachment 499317 [details] [diff] [review]
Latest. Rebased to TR-5705


Requested another SR as I've had to link a new library (rt) in the Solaris build.
Attachment #499317 - Flags: superreview?(edwsmith)

Comment 9

7 years ago
Comment on attachment 499317 [details] [diff] [review]
Latest. Rebased to TR-5705

Is there anything directly testable here, or is usage from upper layers good enough?
Attachment #499317 - Flags: superreview?(edwsmith) → superreview+
(Reporter)

Comment 10

7 years ago
(In reply to comment #9)
> Is there anything directly testable here, or is usage from upper layers good
> enough?

The safepoint selftests directly call VMPI_threadYield(), this should be good enough.

Comment 11

7 years ago
changeset: 5843:891ad70c3012
user:      kpalacz@adobe.com
summary:   Bug 609837 - VMPI threading extensions require a yield-like function (a=swilkin,r+kpalacz,sr+edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/891ad70c3012
(Reporter)

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.