Refactor main C++ function to avoid use "constant" memory and avoid new/delete #55
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hardbyte
approved these changes
Jan 15, 2018
| @@ -139,14 +139,17 @@ extern "C" | |||
| const uint64_t *comp1 = (const uint64_t *) one; | |||
| const uint64_t *comp2 = (const uint64_t *) many; | |||
|
|
|||
| // TODO: Given that k is 10 by default, often 5 in practice, | |||
| // and probably never ever more than 20 or so, the use of a | |||
There was a problem hiding this comment.
This statement isn't (well shouldn't be) correct - anonlink needs to support computing all raw similarity scores.
|
|
||
| if(max_k_scores.size() > k) max_k_scores.pop(); | ||
| // TODO: double precision is overkill for this | ||
| // problem; just use float. |
| @@ -155,39 +158,32 @@ extern "C" | |||
|
|
|||
| for (int j = 0; j < n; j++) { | |||
| const uint64_t *current = comp2 + j * KEYWORDS; | |||
| const uint32_t counts_many_j = counts_many[j]; | |||
There was a problem hiding this comment.
It always suprises me what you have to help the compiler with
There was a problem hiding this comment.
Yeah, this can be more or less important depending on the context. Here it's more about showing intent to fellow programmers than to the compiler.
added 16 commits
February 1, 2018 11:03
…nto hlaw-refactor-cpp
unzvfu
pushed a commit
that referenced
this pull request
Mar 15, 2018
* Skip conversion to cffi char[] unless required * Libraries shouldn't configure logging * Version bump to 0.6.3 * Improvements to benchmark (#58) * Refactor Dice coefficient calculation. * Temporary fiddling with benchmark code. * Calculate and report popcount speed from native code implementation. * Give some values more sensible variable names. * Remove unused import. * Add documentation. * Expand reporting of various measurements. * Comments. * Update README. * Bring test suite up-to-date. * Address Brian's comments. * Update tests; also test native code version. * Print popcount throughput; give some variables better names. * Update README with throughput data. * Refactor main C++ function to avoid use "constant" memory and avoid new/delete (#55) * Refactor main C++ function to avoid use "constant" memory and avoid new/delete. * Refactor Dice coefficient calculation. * Temporary fiddling with benchmark code. * Calculate and report popcount speed from native code implementation. * Give some values more sensible variable names. * Remove unused import. * Add documentation. * Expand reporting of various measurements. * Comments. * Update README. * Bring test suite up-to-date. * Refactor main C++ function to avoid use "constant" memory and avoid new/delete. * Address Brian's comments. * Update tests; also test native code version. * Print popcount throughput; give some variables better names. * Feature build on Travis CI (#61) Run tests with travis ci * Fix #include file name. * Use pytest (#68) * Update README and requirements.txt files. * Add missing line in README. * Use pytest on Jenkins. * Make Jenkins test commands the same as Travis. * Generate test output and coverage data properly. * Move 'checkout scm' command to start of function; remove redundant cleaning code. Fix #65 * Feature use jenkinslibrary (#70) * Update jenkinsfile to use jenkins library. * Reduce the number of OSX build and which node in Jenkinsfile (see #71) * Arbitrary length Dice coefficients (#63) * Refactor main C++ function to avoid use "constant" memory and avoid new/delete. * Implement popcount on (almost) arbitrary length arrays. * First pass at integrating arbitrary length keys. Slows things down a bit. * Refactor Dice coefficient calculation. * Temporary fiddling with benchmark code. * Calculate and report popcount speed from native code implementation. * Give some values more sensible variable names. * Remove unused import. * Add documentation. * Expand reporting of various measurements. * Comments. * Update README. * Bring test suite up-to-date. * Refactor main C++ function to avoid use "constant" memory and avoid new/delete. * Screw everything up by unrolling with C++ templates, apparently. * Magical argument that makes the compiler generate the correct (performant) code. * Address Brian's comments. * Update tests; also test native code version. * Print popcount throughput; give some variables better names. * Make some functions static inline. * Tidy up some expressions. * Put some braces in the right place; make fn inline. * Reinstate comment on origin of popcount assembler. * Make constant a template parameter. * Comment. * Complete version working with multiples of 1024 bits. * Add -march=native compiler option. * Implementation of arbitrary length CLKs. * Fix dumb mistakes in updating array pointer and popcounts. * Tests for arbitrary length popcounts. * Update some comments. * Arbitrary length Dice coefficient. * Rename function. * Move native dicecoeff calculation into its own function. * Add tests for native Dice coefficient calculation. * Move dicecoeff tests to bloommatcher tests; move common bitarray utilities to their own file. * Simplify slow path / reduce branches in fast path. * Adapt entitymatcher to arbitrary length CLK interface. * Remove unused function. * Update README. * Address Brian's comments. * Exit early if filter is zero. * Specialise popcount arrays calls on array length. * Fix performance regression. * Remove storage class specifiers from explicit template specialisations. * Update README and requirements.txt files. * Disable unused function. * Put stars in their proper place. * Add documentation. * Prepare changelog and bump version for release 0.7.0 * Add clkhash as dependency (required for benchmark) Add travis badge to readme
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a low priority PR that came up while looking into data61/anonlink-entity-service#59
The improvement it implements is to use "essentially" constant stack space of size
krather than heap space equivalent to the input lengthn. So we avoid memory allocation/deallocation, heap usage, and it's also somewhat shorter.Also included are some minor syntactical and semantic clean-ups, and some comments for future improvements.
Details:
Previous behaviour was to
all_scoreson the heapall_scoresall_scoresto pick the topkmatches (using the essentially fixed-sized prio queue)all_scoresNow we skip
all_scoresaltogether and just calculate (almost) the scores in turn, inserting each one into the fixed-sized prio queue.