Use Rubinius.lock instead of Channel #1887

Merged
merged 1 commit into from Sep 11, 2012

Conversation

Projects
None yet
2 participants
@ryoqun
Member

ryoqun commented Sep 5, 2012

Hi, As discussed earlier, I made thread use Rubinius.{lock, unlock, synchronize} instead of Channel.

Mostly, this was a straight forward transition.

I commented wherever I thought the code needed to be commented. If there are any other unclear code, please let me know. I'll add comments. :)

Also, because I'm not a Rubinius locking expert, I may be plain wrong.. :p

Any comments are very appreciated!

@dbussink

View changes

vm/builtin/thread.cpp
+ // Wait until the new thread locks the thread object and unlock init_lock_.
+ init_lock_.lock();
+ init_lock_.unlock();
+

This comment has been minimized.

Show comment Hide comment
@dbussink

dbussink Sep 5, 2012

Member

You can't use init_lock_ here directly, since we're after methods that get a gctoken here. That means that the GC might have moved the this pointer. This is also why you see vm->thread-> being used, since that is properly updated when we GC.

Also, why is it locking and unlocking the same lock here? Not really sure I get why

Hmm, looks like the line numbers confused me. So the first comment isn't true. Still wondering a bit about the second one though

@dbussink

dbussink Sep 5, 2012

Member

You can't use init_lock_ here directly, since we're after methods that get a gctoken here. That means that the GC might have moved the this pointer. This is also why you see vm->thread-> being used, since that is properly updated when we GC.

Also, why is it locking and unlocking the same lock here? Not really sure I get why

Hmm, looks like the line numbers confused me. So the first comment isn't true. Still wondering a bit about the second one though

This comment has been minimized.

Show comment Hide comment
@ryoqun

ryoqun Sep 5, 2012

Member

Hi, thanks for comments.

About the second comment, I added more explanation.

For background, I originally wanted to lock the thread object at Thread#setup as @lock was initialized as a new Channel instance previously.

I couldn't do that because it seems that I can't do Rubinius.lock(self) in a thread and do Rubinius.unlock(self) in another thread. So I moved the lock of the thread object to Thread::in_new_thread() from Thread#setup. And, I made the parent thread wait for the child thread to lock the thread object by this lock/unlock pair of init_lock_.

@ryoqun

ryoqun Sep 5, 2012

Member

Hi, thanks for comments.

About the second comment, I added more explanation.

For background, I originally wanted to lock the thread object at Thread#setup as @lock was initialized as a new Channel instance previously.

I couldn't do that because it seems that I can't do Rubinius.lock(self) in a thread and do Rubinius.unlock(self) in another thread. So I moved the lock of the thread object to Thread::in_new_thread() from Thread#setup. And, I made the parent thread wait for the child thread to lock the thread object by this lock/unlock pair of init_lock_.

This comment has been minimized.

Show comment Hide comment
@ryoqun

ryoqun Sep 5, 2012

Member

I think there is a better code snippet for doing what I described in my previous comment probably... If it's true, let me know! I'll update accordingly!

@ryoqun

ryoqun Sep 5, 2012

Member

I think there is a better code snippet for doing what I described in my previous comment probably... If it's true, let me know! I'll update accordingly!

@dbussink

View changes

vm/builtin/thread.cpp
+ // the initialization.
+ // Locking init_lock_ isn't needed anymore, so unlock it.
+ init_lock_.unlock();
+

This comment has been minimized.

Show comment Hide comment
@dbussink

dbussink Sep 5, 2012

Member

One of the things I worry about is the following case. The in_new_thread code could trigger a GC (it uses methods accepting a GCToken), so we might have moved the this pointer when we wait for the lock.

We probably need to copy 'this' into a variable and mark that variable as OnStack so we handle that case correctly. BTW, I'm online on irc if you want to discuss stuff

@dbussink

dbussink Sep 5, 2012

Member

One of the things I worry about is the following case. The in_new_thread code could trigger a GC (it uses methods accepting a GCToken), so we might have moved the this pointer when we wait for the lock.

We probably need to copy 'this' into a variable and mark that variable as OnStack so we handle that case correctly. BTW, I'm online on irc if you want to discuss stuff

@dbussink

This comment has been minimized.

Show comment Hide comment
@dbussink

dbussink Sep 7, 2012

Member

One more thing, shouldn't the logic in Object* Thread::fork(STATE) be applied to Object* Thread::fork_attached(STATE) too? Or maybe we should make attached a boolean argument to the method and merge the two implementations.

Member

dbussink commented Sep 7, 2012

One more thing, shouldn't the logic in Object* Thread::fork(STATE) be applied to Object* Thread::fork_attached(STATE) too? Or maybe we should make attached a boolean argument to the method and merge the two implementations.

@ryoqun

This comment has been minimized.

Show comment Hide comment
@ryoqun

ryoqun Sep 8, 2012

Member

You're correct. I simply overlooked Thread::fork_attached.

For the fix, I chosen a different approach for the refactoring. If it's not good style, I'll update. I tend to be picky for refactoring..

First, I thought it's not good that two functions are doing slightly different thing while their names are similar with the shared verb (fork). So, I renamed Thread::fork_attached to Thread::fork_as_signal_handler, because the fork_attached is only used for signal handlers and will be so for a while. Also, fork_attached is the single specialization of Thread::fork, so it's acceptable to specialize the function much more. In other words, do so to be "for signal handlers", rather than just to be "(be) attached". If the function would be used by several components, it should be named like the current generic fork_attached.

Secondly, I moved signal-handler-specific code from the call site of fork_attached. This is valid because fork_attached became specific by renaming.

Lastly, I created a helper function to be called from both fork and fork_as_signal_handler. It calls pthread_create and lock/unlock init_lock_.

Additionally, in my opinion, the following code at the call site of fork_attached reads much easier:

void SignalHandler::run(STATE) {
  thread_.get()->fork_as_signal_handler(state); // Reader's reasoning: OK, we're specifically forking this thread as a signal handler. We're doing rather unusual thing (setting up the signal handler), so it's plausible that we must fork thread differently in some way.
}

than

void SignalHandler::run(STATE) {
  thread_.get()->fork(state, true); // Reader's reasoning: Ugh, what does the true mean?? Is fork used with true very often?? I need to read the implementation of fork as well..
}

This is based on the general principle that when two or more versions of similar functions are implemented, it's more readable when their differentiated behaviour is encoded into the names of separated functions, rather then the switch parameter of the single function.

Member

ryoqun commented Sep 8, 2012

You're correct. I simply overlooked Thread::fork_attached.

For the fix, I chosen a different approach for the refactoring. If it's not good style, I'll update. I tend to be picky for refactoring..

First, I thought it's not good that two functions are doing slightly different thing while their names are similar with the shared verb (fork). So, I renamed Thread::fork_attached to Thread::fork_as_signal_handler, because the fork_attached is only used for signal handlers and will be so for a while. Also, fork_attached is the single specialization of Thread::fork, so it's acceptable to specialize the function much more. In other words, do so to be "for signal handlers", rather than just to be "(be) attached". If the function would be used by several components, it should be named like the current generic fork_attached.

Secondly, I moved signal-handler-specific code from the call site of fork_attached. This is valid because fork_attached became specific by renaming.

Lastly, I created a helper function to be called from both fork and fork_as_signal_handler. It calls pthread_create and lock/unlock init_lock_.

Additionally, in my opinion, the following code at the call site of fork_attached reads much easier:

void SignalHandler::run(STATE) {
  thread_.get()->fork_as_signal_handler(state); // Reader's reasoning: OK, we're specifically forking this thread as a signal handler. We're doing rather unusual thing (setting up the signal handler), so it's plausible that we must fork thread differently in some way.
}

than

void SignalHandler::run(STATE) {
  thread_.get()->fork(state, true); // Reader's reasoning: Ugh, what does the true mean?? Is fork used with true very often?? I need to read the implementation of fork as well..
}

This is based on the general principle that when two or more versions of similar functions are implemented, it's more readable when their differentiated behaviour is encoded into the names of separated functions, rather then the switch parameter of the single function.

dbussink added a commit that referenced this pull request Sep 11, 2012

Merge pull request #1887 from ryoqun/thread-rubinius-lock
Use Rubinius.lock instead of Channel

@dbussink dbussink merged commit a0c592b into rubinius:master Sep 11, 2012

1 check passed

default The Travis build passed
Details
@ryoqun

This comment has been minimized.

Show comment Hide comment
@ryoqun

ryoqun Sep 12, 2012

Member

Thanks for merging!

@dbussink I'm really thank you for patiently reviewing this pull request!

Member

ryoqun commented Sep 12, 2012

Thanks for merging!

@dbussink I'm really thank you for patiently reviewing this pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment