Skip to content

benchmark: fix pht insert test #184

New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

benchmark: fix pht insert test #184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b69c76
python: wrapper for dht::indexation::Prefix
sim590 Apr 3, 2017
9bb0c47
benchmark: fix Pht insertTest, use wrapped Prefix
sim590 Apr 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion python/opendht.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ cdef inline void lookup_callback(cpp.vector[cpp.shared_ptr[cpp.IndexValue]]* val
v = IndexValue()
v._value = val
vals.append(v)
cbs['lookup'](vals, p.toString())
prefix = Prefix(p.size_)
prefix._prefix.reset(new cpp.Prefix(deref(p)))
cbs['lookup'](vals, prefix)

cdef inline void shutdown_callback(void* user_data) with gil:
cbs = <object>user_data
Expand Down Expand Up @@ -471,6 +473,25 @@ cdef class DhtRunner(_WithID):
ref.Py_DECREF(<object>token._cb['cb'])
# fixme: not thread safe

cdef class Prefix(object):
cdef shared_ptr[cpp.Prefix] _prefix
Copy link
Member

@aberaud aberaud Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why not just cpp.Prefix (like for InfoHash and others) ? __cinit__can be used for initialization. The API doesn't make use of shared_ptr<Prefix>

def __init__(self, int size, bytes val=b''):
cdef cpp.vector[cpp.uint8_t] blob
for c in val:
blob.push_back(c)
self._prefix.reset(new cpp.Prefix(blob, size))
def __str__(self):
return self._prefix.get().toString().decode()
property size_:
def __get__(self):
return self._prefix.get().size_
property content_:
def __get__(self):
return self._prefix.get().toString().decode()
property flags_:
def __get__(self):
return self._prefix.get().flagsToString().decode()

cdef class IndexValue(object):
cdef cpp.shared_ptr[cpp.IndexValue] _value
def __init__(self, InfoHash h=None, cpp.uint64_t vid=0):
Expand Down
6 changes: 6 additions & 0 deletions python/opendht_cpp.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,13 @@ cdef extern from "opendht/indexation/pht.h" namespace "dht::indexation":
cdef cppclass Prefix:
Prefix() except +
Prefix(vector[uint8_t]) except +
Prefix(vector[uint8_t], size_t first) except +
Prefix(Prefix& p) except +
string toString() const
string flagsToString() const
size_t size_
Copy link
Member

@aberaud aberaud Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

IMHO fields should not be exposed here. Even in the C++ code, these fields are only public for convenience but should be made private, since it's dangerous to manipulate them directly (risk of inconsistence, like size_ larger than content_ size()*8 or flags_.size()*8).

edit: this refers to lines 194-196

Copy link
Collaborator

Choose a reason for hiding this comment

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

Totally agreed with that, since flags_ and content_ are internal variable, user shouldn't be able to edit them directly.
As @aberaud point out, there is a risk of inconsistence ( ou just mistake if you [try to] move around those bits )

vector[uint8_t] flags_
vector[uint8_t] content_
ctypedef pair[InfoHash, uint64_t] IndexValue "dht::indexation::Value"
ctypedef map[string, vector[uint8_t]] IndexKey "dht::indexation::Pht::Key"
ctypedef map[string, uint32_t] IndexKeySpec "dht::indexation::Pht::KeySpec"
Expand Down
2 changes: 1 addition & 1 deletion python/tools/benchmark.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def resize_clusters(self, n):
featureArgs.add_argument('--pht', action='store_true', default=False,
help='Launches PHT benchmark test. '\
'Available args for "-t" are: insert. '\
'Timer available by adding "timer" to "-o" args'\
'Timer available by adding "timer" to "-o" args. '\
'Use "-m" option for fixing number of keys to create during the test.')
featureArgs.add_argument('--data-persistence', action='store_true', default=0,
help='Launches data persistence benchmark test. '\
Expand Down
36 changes: 19 additions & 17 deletions python/tools/dht/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,21 +249,22 @@ def _reset(self):
@staticmethod
def lookupCb(vals, prefix):
PhtTest.indexEntries = list(vals)
PhtTest.prefix = prefix.decode()
DhtNetwork.log('Index name: <todo>')
DhtNetwork.log('Leaf prefix:', prefix)
PhtTest.prefix = str(prefix)
Copy link
Member

Choose a reason for hiding this comment

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

Since the goal of this patch is to "fix" the pht test, please also make it use non-static callbacks and fields so we can run multiple instances at the same time. Current design is awful.

# TODO: output index name
# DhtNetwork.Log.log('Index name: <todo>')
DhtNetwork.Log.log('Leaf prefix:', PhtTest.prefix)
for v in vals:
DhtNetwork.log('[ENTRY]:', v)
DhtNetwork.Log.log('[ENTRY]:', v)

@staticmethod
def lookupDoneCb(ok):
DhtNetwork.log('[LOOKUP]:', PhtTest.key, "--", "success!" if ok else "Fail...")
DhtNetwork.Log.log('[LOOKUP]:', PhtTest.key, "--", "success!" if ok else "Fail...")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Log.log, looks like "a lot" of "Log"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaldoran: What do you mean precisely? Dhtnetwork.Log is a class which has methods log, warn and err.

Do you suggest changing Log.log method to Log.debug?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, in order to avoid future mistake. [Log.debug or some other form]

with FeatureTest.lock:
FeatureTest.lock.notify()

@staticmethod
def insertDoneCb(ok):
DhtNetwork.log('[INSERT]:', PhtTest.key, "--", "success!" if ok else "Fail...")
DhtNetwork.Log.log('[INSERT]:', PhtTest.key, "--", "success!" if ok else "Fail...")
with FeatureTest.lock:
FeatureTest.lock.notify()

Expand Down Expand Up @@ -323,10 +324,8 @@ def _insertTest(self):
keyspec = collections.OrderedDict([('foo', NUM_DIG)])
pht = Pht(b'foo_index', keyspec, dht)

DhtNetwork.log('PHT has',
pht.MAX_NODE_ENTRY_COUNT,
'node'+ ('s' if pht.MAX_NODE_ENTRY_COUNT > 1 else ''),
'per leaf bucket.')
DhtNetwork.Log.log('PHT has %s entr%s per leaf bucket.' %
(pht.MAX_NODE_ENTRY_COUNT, 'ies' if pht.MAX_NODE_ENTRY_COUNT > 1 else 'y'))
keys = [{
[_ for _ in keyspec.keys()][0] :
''.join(random.SystemRandom().choice(string.hexdigits)
Expand All @@ -340,7 +339,7 @@ def _insertTest(self):
with FeatureTest.lock:
time_taken = timer(pht.insert, key, IndexValue(random_hash()), PhtTest.insertDoneCb)
if self._timer:
DhtNetwork.log('This insert step took : ', time_taken, 'second')
DhtNetwork.Log.log('This insert step took : ', time_taken, 'second')
FeatureTest.lock.wait()

time.sleep(1)
Expand All @@ -351,15 +350,17 @@ def _insertTest(self):
with FeatureTest.lock:
time_taken = timer(pht.lookup, key, PhtTest.lookupCb, PhtTest.lookupDoneCb)
if self._timer:
DhtNetwork.log('This lookup step took : ', time_taken, 'second')
DhtNetwork.Log.log('This lookup step took : ', time_taken, 'second')
FeatureTest.lock.wait()

all_entries[PhtTest.prefix] = [e.__str__()
for e in PhtTest.indexEntries]
if PhtTest.prefix not in all_entries.keys():
all_entries[PhtTest.prefix] = []
all_entries[PhtTest.prefix].extend([e.__str__() for e in PhtTest.indexEntries])

for p in all_entries.keys():
DhtNetwork.log('All entries under prefix', p, ':')
DhtNetwork.log(all_entries[p])
DhtNetwork.Log.log('All entries under prefix', p, ':')
for e in all_entries[p]:
DhtNetwork.Log.log(e)
PhtTest.drawTrie(all_entries)

##################################
Expand Down Expand Up @@ -496,7 +497,7 @@ def _trigger_dp(self, trigger_nodes, _hash, count=1):
n.run(config=config)
n.bootstrap(self._bootstrap.ip4,
str(self._bootstrap.port))
DhtNetwork.log('Node','['+_hash_str+']',
DhtNetwork.Log.log('Node','['+_hash_str+']',
'started around', _hash.toString().decode()
if n.isRunning() else
'failed to start...'
Expand Down Expand Up @@ -536,6 +537,7 @@ def run(self):
else:
raise NameError("This test is not defined '" + self._test + "'")
except Exception as e:
DhtNetwork.Log.err('Unexpected error')
traceback.print_tb(e.__traceback__)
print(type(e).__name__+':', e, file=sys.stderr)
finally:
Expand Down