Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Dss nonblocking nif #25

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

dizzyd commented Apr 20, 2012

No description provided.

dizzyd added some commits Apr 20, 2012

Move write operations to a non-scheduler thread.
We have observed situations in heavy production load where a few
concurrent writes get backed up behind compactions in such a way that
work-stealing appears to stop working and the VM can make no further
process until the blocked schedulers resume.

Read/iteration functionality remains on the scheduler threads, since
they should never block due to the append-only design of LevelDB.

@slfritchie slfritchie and 1 other commented on an outdated diff Apr 20, 2012

c_src/eleveldb.cc
- // TODO: Why does the API want a WriteBatch* versus a ref?
- leveldb::Status status = handle->db->Write(opts, &batch);
- if (status.ok())
- {
- return ATOM_OK;
- }
- else
- {
- return error_tuple(env, ATOM_ERROR_DB_WRITE, status);
- }
+ // Identify the calling PID and make that available to the worker
+ ErlNifPid caller;
+ enif_self(env, &caller);
+ handle->write_caller = enif_make_pid(handle->write_env, &caller);
+
+ // Construct a reference that will be used to identify this specific
@slfritchie

slfritchie Apr 20, 2012

Contributor

I don't know enough about the ref-based message receive optimization to answer this question so late at night, but ... it's my tiny understanding of the optimization that the ref is created by the Erlang side of the world, then sent in a message in a particular position in a tuple?

@dizzyd

dizzyd Apr 20, 2012

Contributor

You are completely right -- confirmed it with Ulf this morning. Pushed a change, accordingly. Good catch!

@ghost ghost assigned jonmeredith Apr 20, 2012

@jonmeredith jonmeredith commented on the diff Apr 20, 2012

c_src/eleveldb.cc
- // TODO: Why does the API want a WriteBatch* versus a ref?
- leveldb::Status status = handle->db->Write(opts, &batch);
- if (status.ok())
- {
- return ATOM_OK;
- }
- else
- {
- return error_tuple(env, ATOM_ERROR_DB_WRITE, status);
- }
+ // Identify the calling PID and make that available to the worker
@jonmeredith

jonmeredith Apr 20, 2012

Contributor

Would benefit from a comment to explain why the Eterm gymnastics is being used (erring on the side of caution)

@jonmeredith jonmeredith commented on an outdated diff Apr 20, 2012

c_src/eleveldb.cc
- }
+ // Identify the calling PID and make that available to the worker
+ ErlNifPid caller;
+ enif_self(env, &caller);
+ handle->write_caller = enif_make_pid(handle->write_env, &caller);
+
+ // Construct a reference that will be used to identify this specific
+ // request back to the caller and take advantage of optimizations
+ // for long message queues
+ //
+ // PARANOID: copy the ref into the caller's env; not clear if this
+ // is strictly necessary
+ handle->write_ref = enif_make_ref(handle->write_env);
+ ERL_NIF_TERM ref = enif_make_copy(env, handle->write_ref);
+
+ // Kick the writer thread and wait for a response
@jonmeredith

jonmeredith Apr 20, 2012

Contributor

Clarify to say that it's the erlang code that will wait for the response

@jonmeredith jonmeredith commented on the diff Apr 20, 2012

c_src/eleveldb.cc
+
+ ERL_NIF_TERM result;
+ if (status.ok())
+ {
+ result = enif_make_tuple2(handle->write_env, handle->write_ref, ATOM_OK);
+ }
+ else
+ {
+ ERL_NIF_TERM error = error_tuple(handle->write_env, ATOM_ERROR_DB_WRITE, status);
+ result = enif_make_tuple2(handle->write_env, handle->write_ref, error);
+ }
+
+ // Send back the result of the write
+ ErlNifPid caller;
+ enif_get_local_pid(handle->write_env, handle->write_caller, &caller);
+ enif_send(NULL, &caller, handle->write_env, result);
@jonmeredith

jonmeredith Apr 20, 2012

Contributor

Docs say you can only call this from a non-NIF called thread on the SMP emulator. We need to add similar protection in the NIF init code that we have in innostore https://github.com/basho/innostore/blob/master/c_src/innostore_drv.c#L188

Contributor

jonmeredith commented Apr 20, 2012

Code looks correct - I'm going to run some benchmarks/load testing to check for any leaks/nastiness and I'm going to double-check the error paths one last time.

@jonmeredith jonmeredith commented on the diff Apr 20, 2012

c_src/eleveldb.cc
- return error_tuple(env, ATOM_ERROR_DB_WRITE, status);
- }
+ // Identify the calling PID and make that available to the worker
+ ErlNifPid caller;
+ enif_self(env, &caller);
+ handle->write_caller = enif_make_pid(handle->write_env, &caller);
+
+ // Save the write-reference for the response back to the caller;
+ // enables us to use receive optimizations in the VM
+ handle->write_ref = enif_make_copy(handle->write_env, writeref);
+
+ // Kick the writer thread and wait for a response
+ enif_cond_signal(handle->write_cv);
+ enif_mutex_unlock(handle->write_lock);
+
+ // Return the reference
@jonmeredith

jonmeredith Apr 20, 2012

Contributor

Reference is passed in now

Contributor

slfritchie commented Mar 26, 2013

Can this issue be closed, given all the Riak 1.3.0 work on the NIF for eleveldb?

@matthewvon matthewvon closed this Aug 14, 2013

@seancribbs seancribbs deleted the dss-nonblocking-nif branch Apr 1, 2015

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