Thread-safety for hashtable #876

camillescott · 2015-03-18T19:22:18Z

The multithreading PR has gotten unwieldy, so this is part of the process to break it down into more manageable chunks. This set of commits adds an array of locks to Hashtable with are indexed by the hashvalue, allowing a thread to lock only a small block when writing. It also introduces a new function meant specifically for thread-safe counting, to avoid overhead under single-threaded cases and maintain backwards compatibility.

camillescott · 2015-03-18T21:13:21Z

Is it mergeable?
Did it pass the tests?
If it introduces new functionality in scripts/ is it tested?
Check for code coverage with make clean diff-cover
Is it well formatted? Look at make pep8, make diff_pylint_report,
make cppcheck, and make doc output. Use make format and manual
fixing as needed.
Did it change the command-line interface? Only additions are allowed
without a major version increment. Changing file formats also requires a
major version number increment.
Is it documented in the ChangeLog?
http://en.wikipedia.org/wiki/Changelog#Format
Was a spellchecker run on the source code and documentation after
changes were made?

camillescott · 2015-03-18T21:32:50Z

Jenkins, test this please, or we'll be having another stern talk.

camillescott · 2015-03-18T21:50:51Z

You ready to test this please, Jenkins?

camillescott · 2015-03-18T21:57:03Z

@mr-c @ctb @luizirber thoughts? is the testing robust enough? does a separate function (count_ts) make sense?

note: i've also tested this by monkeypatching count_ts as count and running all tests; it works perfectly.

camillescott · 2015-03-23T04:37:05Z

Have you been tippin' the bottle again? Jenkins, test this please!

mr-c · 2015-03-23T15:19:02Z

khmer/_khmermodule.cc

@@ -1541,10 +1618,14 @@ static PyMethodDef khmer_counting_methods[] = {
    { "hashsizes", (PyCFunction)hash_get_hashsizes, METH_VARARGS, "" },
    { "set_use_bigcount", (PyCFunction)hash_set_use_bigcount, METH_VARARGS, "" },
    { "get_use_bigcount", (PyCFunction)hash_get_use_bigcount, METH_VARARGS, "" },
+    { "init_threadsafe", (PyCFunction)hash_init_threadsafe, METH_VARARGS, "Initialize the table lock array" },
+    { "n_lock_blocks", (PyCFunction)hash_n_lock_blocks, METH_VARARGS, "Number of blocks in the lock array" },


Shouldn't this be _ prefixed to indicate that it is a private/testing function?

The number of lock blocks is passed when calling init_threadsafe. It has a default value that is hardcoded, but the user can override it; there isn't necessary anything private or testing-specific about it. I will make it const const though.

mr-c · 2015-03-23T16:09:44Z

The current count() function https://github.com/ged-lab/khmer/blob/feature/thread_safety/lib/counting.hh#L175 looks to be half thread-safe.

Is there really that much overhead when single-threaded?

I'd rather see the extra thread safety added to count(). If there is significant slowdown when single threaded then we can put all the spinlocks (old and new) inside if statements.

camillescott · 2015-03-23T16:16:47Z

@mr-c: I'm not sure on the overhead. Some recent reading I did suggests that CAS operations have significant overhead when called repeatedly (for example: http://joeduffyblog.com/2009/01/08/some-performance-implications-of-cas-operations/). However, I'll need to run some benchmarks to figure out the effects of this particular implementation.

mr-c · 2015-03-23T16:22:51Z

(Also, thanks!)

…nge around variable names to be more sensible; change default beahvior for number of locks and minimum number of locks

ctb · 2015-03-27T09:49:54Z

lib/counting.hh

@@ -94,6 +94,17 @@ public:
        return _counts;
    }

+    virtual void init_threadstuff(unsigned int n_table_locks=DEFAULT_TABLE_LOCKS) {
+        if(!_threadsafe) {
+            _n_table_locks = n_table_locks < 4 ? 4 : n_table_locks;


ctb · 2015-03-27T10:00:08Z

+1 on @mr-c's comment regarding adding thread-safety to count instead of building a new function. Other than that, and a few questions, looks good and straightforward. The testing is OK although I doubt it's going to hit any problematic cases even w/o thread safety (might be interesting to test this by disabling thread safety and running that test a few hundred times... does it ever break?)

I'm assuming that the real purpose of this addition is not to use the Python interface, right? The Python interface is not going to give you a massive speed improvement here, I don't think!

camillescott · 2015-03-30T19:31:59Z

(Putting some of our Friday discussion here just to have it written down somewhere)

The testing is OK although I doubt it's going to hit any problematic cases even w/o thread safety (might be interesting to test this by disabling thread safety and running that test a few hundred times... does it ever break?)

It seems that it takes longer for the Python interpreter to load in the next bit of bytecode and execute it than it does for the count_ts function to be called -- I couldn't get this to trip from Python land.

I'm assuming that the real purpose of this addition is not to use the Python interface, right? The Python interface is not going to give you a massive speed improvement here, I don't think!

Correct -- I was trying to figure out a way to test it with our current suite. At this point, it looks like more thorough testing will have to wait for the full multithreading PR.

camillescott added 4 commits March 18, 2015 15:18

Add thread-safety to hashtable

eaece55

Expose threadsafety methods to python

f479f2f

Expose count_ts to python

70a482e

Add tests for count_ts

4ef28c8

Update ChangeLog; fix pep8

65d58bc

Add a check to init_threadsafe to avoid redudant realloc

988e01b

camillescott added 3 commits March 20, 2015 16:22

Add function overload for count_ts

d2e21a9

Add getter and tests for number of lock blocks

ae0f03e

Fix dumb homopolymer test

d2fbde4

Merge in master, resolve and update ChangeLog

34851aa

mr-c reviewed Mar 23, 2015
View reviewed changes

Change block_size parameter to number of table locks for clarity; cha…

6518f94

…nge around variable names to be more sensible; change default beahvior for number of locks and minimum number of locks

ctb reviewed Mar 27, 2015
View reviewed changes

Merge in master and resolve ChangeLog conflict

b2eaa5f

mr-c added this to the 1.4+ milestone May 13, 2015

Update with new CPython standards, re architect slightly

128f54a

camillescott mentioned this pull request Sep 4, 2015

test_load_graph_multithread() in test_scripts.py finds different unique k-mers #1248

Open

standage requested review from SensibleSalmon and removed request for SensibleSalmon February 4, 2018 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread-safety for hashtable #876

Thread-safety for hashtable #876

camillescott commented Mar 18, 2015

camillescott commented Mar 18, 2015

camillescott commented Mar 18, 2015

camillescott commented Mar 18, 2015

camillescott commented Mar 18, 2015

camillescott commented Mar 23, 2015

mr-c Mar 23, 2015

camillescott Mar 23, 2015

mr-c commented Mar 23, 2015

camillescott commented Mar 23, 2015

mr-c commented Mar 23, 2015

ctb Mar 27, 2015

ctb commented Mar 27, 2015

camillescott commented Mar 30, 2015

Thread-safety for hashtable #876

Are you sure you want to change the base?

Thread-safety for hashtable #876

Conversation

camillescott commented Mar 18, 2015

camillescott commented Mar 18, 2015

camillescott commented Mar 18, 2015

camillescott commented Mar 18, 2015

camillescott commented Mar 18, 2015

camillescott commented Mar 23, 2015

mr-c Mar 23, 2015

Choose a reason for hiding this comment

camillescott Mar 23, 2015

Choose a reason for hiding this comment

mr-c commented Mar 23, 2015

camillescott commented Mar 23, 2015

mr-c commented Mar 23, 2015

ctb Mar 27, 2015

Choose a reason for hiding this comment

ctb commented Mar 27, 2015

camillescott commented Mar 30, 2015