rts/itimer/pthread: Stop timer when ticker is stopped
ClosedPublic

Authored by bgamari on Apr 21 2016, 6:07 AM.

Details

Summary

This reworks the pthread-based itimer implementation to disarm the timer
when events aren't needed. Thanks to hsyl20 for the nice design.

Test Plan

Validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bgamari updated this revision to Diff 7343.Apr 21 2016, 6:07 AM
bgamari retitled this revision from to rts/itimer/pthread: Stop timer when ticker is stopped.
bgamari updated this object.
bgamari edited the test plan for this revision. (Show Details)
bgamari added reviewers: hsyl20, simonmar.
bgamari updated the Trac tickets for this revision.
bgamari edited edge metadata.Apr 21 2016, 6:13 AM
bgamari updated the Trac tickets for this revision.
bgamari updated this revision to Diff 7345.Apr 21 2016, 7:26 AM
bgamari edited edge metadata.

Rip out STOPPING state

It served no purposed but to complicate the thread-safety story.

simonmar requested changes to this revision.Apr 21 2016, 7:33 AM
simonmar edited edge metadata.

please use ACQUIRE_LOCK() and RELEASE_LOCK() instead of the pthread primitives, because these have extra debugging enabled with -DDEBUG.

rts/posix/itimer/Pthread.c
136

I think you want to move this lock up to before the switch, otherwise the state may change before blocking on the condition variable, and you'll deadlock. Locks are hard!

165

Also exit? we probably can't continue if these fail

This revision now requires changes to proceed.Apr 21 2016, 7:33 AM
bgamari updated this revision to Diff 7348.Apr 21 2016, 8:03 AM
bgamari edited edge metadata.
  • rts/itimer/pthread: Stop timer when ticker is stopped
  • Simplify
simonmar accepted this revision.Apr 21 2016, 8:20 AM
simonmar edited edge metadata.

Sure. I'm not sure if it's simpler because there are 4 combinations of {stopped,exited} instead of 3 states, but as you say there are fewer transitions. I don't feel strongly about it.

Need to remove the extra ITimerState declaration, update the comment, and add calls to stg_exit.

This revision is now accepted and ready to land.Apr 21 2016, 8:20 AM
bgamari updated this revision to Diff 7349.Apr 21 2016, 8:20 AM
bgamari edited edge metadata.
  • rts/itimer/pthread: Stop timer when ticker is stopped
  • Simplify
bgamari updated this object.Apr 21 2016, 8:20 AM
bgamari edited edge metadata.
hsyl20 added inline comments.Apr 21 2016, 8:27 AM
rts/posix/itimer/Pthread.c
201

We could implement "wait" with a pthread_join. Do we want it?

202

Maybe add ASSERT(!exited) here to avoid trying to start an already exited ticker (with freed mutex/cond).

bgamari updated this revision to Diff 7350.Apr 21 2016, 8:38 AM
  • rts/itimer/pthread: Stop timer when ticker is stopped
  • Simplify
bgamari updated this revision to Diff 7351.Apr 21 2016, 9:30 AM
bgamari edited edge metadata.
  • rts/itimer/pthread: Stop timer when ticker is stopped
  • Simplify
  • Ensure that ticker terminates
bgamari marked 3 inline comments as done.Apr 21 2016, 9:31 AM
bgamari added inline comments.
rts/posix/itimer/Pthread.c
201

In fact we must, since otherwise we may try to run a tick after the RTS has begun shutting down.

202

Fair point.

bgamari updated this revision to Diff 7352.Apr 21 2016, 9:32 AM
bgamari marked an inline comment as done.
bgamari edited edge metadata.
  • rts/itimer/pthread: Stop timer when ticker is stopped
  • Simplify
  • Ensure that ticker terminates
  • Add assert
simonmar requested changes to this revision.Apr 30 2016, 9:59 AM
simonmar edited edge metadata.

Please use ACQUIRE_LOCK and RELEASE_LOCK instead of the raw pthread functions. In general I think it would be better to use the generic RTS interfaces, even inside rts/posix, to avoid duplicating things and to take advantage of the extra DEBUG checks.

rts/posix/itimer/Pthread.c
169–176

Just use createOSThread() here?

This revision now requires changes to proceed.Apr 30 2016, 9:59 AM
bgamari added inline comments.May 1 2016, 7:01 AM
rts/posix/itimer/Pthread.c
169–176

Sadly that won't fly since createOSThread immediately detaches, which renders the thread un-joinable.

bgamari updated this revision to Diff 7430.May 1 2016, 7:02 AM
bgamari edited edge metadata.
  • Use RTS synchronization primitives

@simonmar, how does this look? I don't use the RTS's thread creation utilities for the reason mentioned earlier but at least we now use the synchronization primitives.

simonmar accepted this revision.May 1 2016, 10:13 AM
simonmar edited edge metadata.

Yeah, looks great, thanks. I would also add a comment about why we don't use createOSThread().

This revision is now accepted and ready to land.May 1 2016, 10:13 AM
bgamari updated this revision to Diff 7431.May 1 2016, 10:37 AM
bgamari edited edge metadata.

Add comment

bgamari updated this object.May 1 2016, 10:40 AM
bgamari edited edge metadata.
This revision was automatically updated to reflect the committed changes.