Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

spinlock for GC#1447

Merged
andralex merged 4 commits intodlang:masterfrom
MartinNowak:spinLock
Mar 8, 2016
Merged

spinlock for GC#1447
andralex merged 4 commits intodlang:masterfrom
MartinNowak:spinLock

Conversation

@MartinNowak
Copy link
Member

  • less overhead than pthread_mutex
  • uses test and test-and-set algorithm with configurable backoff

Had to reopen spinlock for GC by MartinNowak · Pull Request #1153 after force pushing, please look the existing review before repeating any questions.

chart

@MartinNowak MartinNowak mentioned this pull request Dec 1, 2015
@MartinNowak MartinNowak added the GC garbage collector label Dec 1, 2015
@MartinNowak
Copy link
Member Author

Closing again until fixed, seems to deadlock a dmd test.

@MartinNowak
Copy link
Member Author

Ready now, after fixing a dead-lock caused by onOutOfMemoryError.
This still provides a substantial speed-up, in particular for concurrent code.

@MartinNowak
Copy link
Member Author

updated to resolve merge conflict

{
for (size_t n; atomicLoad!(MemoryOrder.raw)(val); ++n)
yield(n);
if (cas(&val, false, true))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you want to optimistically start with the cas?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, hardly measurable.

@andralex
Copy link
Member

I was to pull this, but raises enough flags to warrant another pass.

- the compiler doesn't recognize this as super call
  and will use dynamic dispatch leading to an infinite loop
- less overhead than pthread_mutex
- uses test and test-and-set algorithm with configurable backoff
- always use runLocked which now unlocks on error
- check for recursive GC entrance during finalization
  inFinalizer can be thread local b/c we're finalizing in the
  same thread that currently holds the lock
- all trace handlers should be @nogc but that's currently out of
  scope b/c it requires to rewrite all backtrace and demangle code
@MartinNowak
Copy link
Member Author

Update to address the review points.
spinlock
Also rebenchmarked, as before this is mostly an improvement for multi-threaded programms (see conmsg).


void lock()
{
if (cas(&val, size_t(0), size_t(1)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think restructuring this function as a do/while so there's only one cas() might generate tighter code. But not urgent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid accessing the contention field.

@andralex
Copy link
Member

andralex commented Mar 8, 2016

Auto-merge toggled on

andralex added a commit that referenced this pull request Mar 8, 2016
@andralex andralex merged commit 51576e7 into dlang:master Mar 8, 2016
@MartinNowak MartinNowak deleted the spinLock branch March 9, 2016 02:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

GC garbage collector

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants