Skip to content

Commit

Permalink
Use per extension C-API locks and setup blacklist mechanism
Browse files Browse the repository at this point in the history 
This allows for using a per extension C-API lock instead of a global
lock. Each time an extension entry point is loaded (the Init_ function),
we setup a new lock index. We store this in the native method frame so
any methods defined will pick up the lock index.

This lock index is then stored for the native method and used when
locking around a C extension call. We use the value of 0 to indicate an
extension doesn't need locking if -Xcapi.lock is not enabled.

If -Xcapi.lock is enabled, each extension gets it's own lock through
this mechanism. Except for this general strategy, we also have a
blacklist which contains known bad extensions that always need a lock
around them. A known example of this is NKF, which tracks all it's
internal state in global C variables.

The blacklist mechanism allows for an easy way to add more troublesome
extensions without having to enabled the C-API lock for everything.
  • Loading branch information
@dbussink
dbussink committed May 13, 2013
1 parent f7105b9 commit 0791447
Show file tree
Hide file tree
Showing 8 changed files with 105 additions and 44 deletions.
22 changes: 17 additions & 5 deletions vm/builtin/nativemethod.cpp
View file
Expand Up @@ -46,6 +46,13 @@ namespace rubinius {
return native_method_environment.get();
}

NativeMethodFrame::NativeMethodFrame(NativeMethodFrame* prev, NativeMethod* method)
: previous_(prev)
, capi_lock_index_(method ? method->capi_lock_index() : 0)
, check_handles_(false)
, block_(Qnil)
{}

NativeMethodFrame::~NativeMethodFrame() {
flush_cached_data();
handles_.deref_all();
Expand Down Expand Up @@ -657,8 +664,7 @@ namespace rubinius {
}
}


NativeMethodFrame nmf(env->current_native_frame());
NativeMethodFrame nmf(env->current_native_frame(), nm);
CallFrame* call_frame = ALLOCA_CALLFRAME(0);
call_frame->previous = previous;
call_frame->constant_scope_ = 0;
Expand Down Expand Up @@ -735,11 +741,12 @@ namespace rubinius {
RUBINIUS_METHOD_NATIVE_RETURN_HOOK(state, mod, args.name(), call_frame);
#endif

LEAVE_CAPI(state);

env->set_current_call_frame(saved_frame);
env->set_current_native_frame(nmf.previous());
ep.pop(env);

LEAVE_CAPI(state);
OnStack<1> os_ret(state, ret);

// Handle any signals that occurred while the native method
Expand All @@ -753,14 +760,18 @@ namespace rubinius {
String* library, Symbol* name, Pointer* ptr) {
void* func = ptr->pointer;

int capi_lock_index = state->shared().capi_lock_index(name->debug_str(state));

return NativeMethod::create(state, library, G(rubinius),
name, func,
Fixnum::from(INIT_FUNCTION));
Fixnum::from(INIT_FUNCTION),
capi_lock_index);
}

NativeMethod* NativeMethod::create(State* state, String* file_name,
Module* module, Symbol* method_name,
void* func, Fixnum* arity)
void* func, Fixnum* arity,
int capi_lock_index)
{
NativeMethod* nmethod = state->new_object<NativeMethod>(G(nmethod));

Expand Down Expand Up @@ -798,6 +809,7 @@ namespace rubinius {
nmethod->inliners_ = 0;
nmethod->prim_index_ = -1;
nmethod->custom_call_site_ = false;
nmethod->capi_lock_index_ = capi_lock_index;

return nmethod;
}
Expand Down
21 changes: 14 additions & 7 deletions vm/builtin/nativemethod.hpp
View file
Expand Up @@ -160,6 +160,7 @@ namespace rubinius {
/** HandleSet to Objects used in this Frame. */
capi::HandleSet handles_;

int capi_lock_index_;
bool check_handles_;

/** Handle for the block passed in **/
Expand All @@ -175,12 +176,7 @@ namespace rubinius {
VALUE method_;

public:
NativeMethodFrame(NativeMethodFrame* prev)
: previous_(prev)
, check_handles_(false)
, block_(Qnil)
{}

NativeMethodFrame(NativeMethodFrame* prev, NativeMethod* method);
~NativeMethodFrame();

public: /* Interface methods */
Expand All @@ -204,6 +200,10 @@ namespace rubinius {
/** Updates cached data with changes to the Ruby objects. */
void update_cached_data();

int capi_lock_index() {
return capi_lock_index_;
}

public: /* Accessors */

/** HandleSet to Objects used in this Frame. */
Expand Down Expand Up @@ -303,6 +303,8 @@ namespace rubinius {
/** Function object that implements this method. */
void* func_;

int capi_lock_index_;

public: /* Accessors */

/** Arity of the method within. @see Arity. */
Expand All @@ -314,6 +316,10 @@ namespace rubinius {
/** Module on which created. */
attr_accessor(module, Module);

int capi_lock_index() {
return capi_lock_index_;
}

public: /* Ruby bookkeeping */

/** Statically held object type. */
Expand All @@ -340,7 +346,8 @@ namespace rubinius {
*/
static NativeMethod* create(State* state, String* file_name,
Module* module, Symbol* method_name,
void* func, Fixnum* arity);
void* func, Fixnum* arity,
int capi_lock_index);

public: /* Class Interface */

Expand Down
12 changes: 3 additions & 9 deletions vm/capi/capi.cpp
View file
Expand Up @@ -27,13 +27,6 @@
#include "dispatch.hpp"
#include "capi/capi.hpp"
#include "capi/18/include/ruby.h"
#include <string>
#include <vector>
#include <tr1/unordered_map>

#ifdef RBX_WINDOWS
#include <malloc.h>
#endif

namespace rubinius {
namespace capi {
Expand Down Expand Up @@ -389,7 +382,7 @@ namespace rubinius {
NativeMethod* nm = NativeMethod::create(env->state(),
nil<String>(), env->state()->vm()->shared.globals.rubinius.get(),
env->state()->symbol("call"), cb,
Fixnum::from(arity));
Fixnum::from(arity), 0);

nm->set_ivar(env->state(), env->state()->symbol("cb_data"),
env->get_object(cb_data));
Expand Down Expand Up @@ -682,7 +675,8 @@ extern "C" {
NativeMethod* method = NULL;
method = NativeMethod::create(state, String::create(state, file),
module, method_name,
(void*)fptr, Fixnum::from(arity));
(void*)fptr, Fixnum::from(arity),
env->current_native_frame()->capi_lock_index());

Symbol* visibility;

Expand Down
23 changes: 11 additions & 12 deletions vm/capi/thread.cpp
View file
Expand Up @@ -165,10 +165,16 @@ extern "C" {
}

Object* run_function(STATE) {

NativeMethodEnvironment* env = NativeMethodEnvironment::get();

NativeMethodFrame nmf(0);
Thread* self = state->vm()->thread.get();
NativeMethod* nm = as<NativeMethod>(self->locals_aref(state, state->symbol("function")));
Pointer* ptr = as<Pointer>(self->locals_aref(state, state->symbol("argument")));

self->locals_remove(state, state->symbol("function"));
self->locals_remove(state, state->symbol("argument"));

NativeMethodFrame nmf(0, nm);
CallFrame cf;
cf.previous = 0;
cf.constant_scope_ = 0;
Expand All @@ -184,13 +190,6 @@ extern "C" {
env->set_current_call_frame(&cf);
env->set_current_native_frame(&nmf);

Thread* self = state->vm()->thread.get();
NativeMethod* nm = as<NativeMethod>(self->locals_aref(state, state->symbol("function")));
Pointer* ptr = as<Pointer>(self->locals_aref(state, state->symbol("argument")));

self->locals_remove(state, state->symbol("function"));
self->locals_remove(state, state->symbol("argument"));

nmf.setup(
env->get_handle(self),
env->get_handle(cNil),
Expand All @@ -213,12 +212,12 @@ extern "C" {
ret = env->get_object(nm->func()(ptr->pointer));
}

LEAVE_CAPI(state);

env->set_current_call_frame(saved_frame);
env->set_current_native_frame(nmf.previous());
ep.pop(env);

LEAVE_CAPI(state);

return ret;
}

Expand All @@ -229,7 +228,7 @@ extern "C" {
NativeMethod* nm = NativeMethod::create(state,
String::create(state, file), G(thread),
state->symbol(name), (void*)func,
Fixnum::from(1));
Fixnum::from(1), 0);

Pointer* ptr = Pointer::create(state, arg);

Expand Down
5 changes: 4 additions & 1 deletion vm/gc/finalize.cpp
View file
Expand Up @@ -257,7 +257,7 @@ namespace rubinius {
if(process_item_->finalizer) {

NativeMethodEnvironment* env = NativeMethodEnvironment::get();
NativeMethodFrame nmf(0);
NativeMethodFrame nmf(0, 0);
CallFrame* call_frame = ALLOCA_CALLFRAME(0);
call_frame->previous = 0;
call_frame->constant_scope_ = 0;
Expand All @@ -274,6 +274,9 @@ namespace rubinius {

// Register the CallFrame, because we might GC below this.
state->set_call_frame(call_frame);

nmf.setup(Qnil, Qnil, Qnil, Qnil);

(*process_item_->finalizer)(state, process_item_->object);

state->set_call_frame(0);
Expand Down
42 changes: 37 additions & 5 deletions vm/shared_state.cpp
View file
Expand Up @@ -16,6 +16,7 @@
#include "builtin/randomizer.hpp"
#include "builtin/array.hpp"
#include "builtin/thread.hpp"
#include "builtin/nativemethod.hpp"

#include <iostream>
#include <iomanip>
Expand Down Expand Up @@ -62,6 +63,7 @@ namespace rubinius {
}

hash_seed = Randomizer::random_uint32();
initialize_capi_black_list();
}

SharedState::~SharedState() {
Expand Down Expand Up @@ -180,8 +182,8 @@ namespace rubinius {
global_cache->reset();
onig_lock_.init();
ruby_critical_lock_.init();
capi_lock_.init();
capi_ds_lock_.init();
capi_locks_lock_.init();
capi_constant_lock_.init();
auxiliary_threads_->init();

Expand Down Expand Up @@ -255,17 +257,42 @@ namespace rubinius {
}

void SharedState::enter_capi(STATE, const char* file, int line) {
if(use_capi_lock_) {
capi_lock_.lock(state->vm(), file, line);
NativeMethodEnvironment* env = NativeMethodEnvironment::get();
if(int lock_index = env->current_native_frame()->capi_lock_index()) {
capi_locks_[lock_index - 1]->lock(state->vm(), file, line);
}
}

void SharedState::leave_capi(STATE) {
if(use_capi_lock_) {
capi_lock_.unlock(state->vm());
NativeMethodEnvironment* env = NativeMethodEnvironment::get();
if(int lock_index = env->current_native_frame()->capi_lock_index()) {
capi_locks_[lock_index - 1]->unlock(state->vm());
}
}

int SharedState::capi_lock_index(std::string name) {
utilities::thread::SpinLock::LockGuard guard(capi_locks_lock_);
int existing = capi_lock_map_[name];
if(existing) return existing;

CApiBlackList::const_iterator blacklisted = capi_black_list_.find(name);

// Only disable locks if we have capi locks disabled
// and library is not in the blacklist.
if(!use_capi_lock_ && blacklisted == capi_black_list_.end()) {
capi_lock_map_[name] = 0;
return 0;
}

Mutex* lock = new Mutex();
capi_locks_.push_back(lock);

// We use a 1 offset index, so 0 can indicate no lock used
int lock_index = capi_locks_.size();
capi_lock_map_[name] = lock_index;
return lock_index;
}

void SharedState::setup_capi_constant_names() {
capi_constant_name_map_.resize(cCApiMaxConstant + 1);

Expand Down Expand Up @@ -345,4 +372,9 @@ namespace rubinius {
}

}

#define CAPI_BLACK_LIST(name) capi_black_list_.insert(std::string("Init_" # name))
void SharedState::initialize_capi_black_list() {
CAPI_BLACK_LIST(nkf);
}
}
20 changes: 17 additions & 3 deletions vm/shared_state.hpp
View file
Expand Up @@ -22,6 +22,10 @@

#include "capi/capi_constants.h"

#include <vector>
#include <tr1/unordered_map>
#include <tr1/unordered_set>

#ifdef RBX_WINDOWS
#include <winsock2.h>
#endif
Expand Down Expand Up @@ -51,6 +55,10 @@ namespace rubinius {
class QueryAgent;
class Environment;

typedef std::tr1::unordered_set<std::string> CApiBlackList;
typedef std::vector<Mutex*> CApiLocks;
typedef std::tr1::unordered_map<std::string, int> CApiLockMap;

typedef std::vector<std::string> CApiConstantNameMap;
typedef std::tr1::unordered_map<int, capi::Handle*> CApiConstantHandleMap;

Expand Down Expand Up @@ -102,13 +110,16 @@ namespace rubinius {
utilities::thread::Mutex ruby_critical_lock_;
pthread_t ruby_critical_thread_;

Mutex capi_lock_;
bool use_capi_lock_;

utilities::thread::SpinLock capi_ds_lock_;
utilities::thread::SpinLock capi_locks_lock_;
utilities::thread::SpinLock capi_constant_lock_;
utilities::thread::SpinLock llvm_state_lock_;

CApiBlackList capi_black_list_;
CApiLocks capi_locks_;
CApiLockMap capi_lock_map_;

bool use_capi_lock_;
int primitive_hits_[Primitives::cTotalPrimitives];

public:
Expand Down Expand Up @@ -288,6 +299,8 @@ namespace rubinius {
return capi_constant_lock_;
}

int capi_lock_index(std::string name);

utilities::thread::SpinLock& llvm_state_lock() {
return llvm_state_lock_;
}
Expand Down Expand Up @@ -326,6 +339,7 @@ namespace rubinius {
return capi_constant_handle_map_;
}

void initialize_capi_black_list();
};
}

Expand Down
4 changes: 2 additions & 2 deletions vm/test/test_nativemethod.hpp
View file
Expand Up @@ -18,7 +18,7 @@ class TestNativeMethod : public CxxTest::TestSuite, public VMTest {
}

void test_native_method_frame_get_object() {
NativeMethodFrame nmf(NULL);
NativeMethodFrame nmf(NULL, NULL);
String* str = String::create(state, "test");
Integer* id = str->id(state);
VALUE handle = nmf.get_handle(state, str);
Expand All @@ -27,7 +27,7 @@ class TestNativeMethod : public CxxTest::TestSuite, public VMTest {
}

NativeMethodEnvironment* create_native_method_environment() {
NativeMethodFrame* nmf = new NativeMethodFrame(NULL);
NativeMethodFrame* nmf = new NativeMethodFrame(NULL, NULL);
CallFrame* cf = new CallFrame;
NativeMethodEnvironment* nme = new NativeMethodEnvironment(state);

Expand Down

0 comments on commit 0791447

Please sign in to comment.