[NARS1] [estess] Improve performance of quicksort (and in turn string… #86
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
…pool garbage collection in YottaDB) by choosing a better pivot YottaDB#85 The primary enhancement is to stpg_sort.c. Previously, the pivot was chosen as the median of the left, right and (left+right)/2 indices in the array. This is the normally recommended pivot rule for quicksort. But it was observed to give us a pivot that ended up almost in one corner of the array (i.e. too close to left or too close to right). This in turn caused quicksort to degenerate into an O(n^2) algorithm which showed its colors when a garbage collection had to run with thousands of items. The pivot is now chosen as the median of 9 equally-spaced numbers in the array spanning [left,right] indices. And this was observed to give us a pivot that ended up almost in the midpoint of the array (45% most of the time) thus enabling quicksort to run in O(nlogn). With these changes, a garbage collection that used to take 83 seconds took 0.5 seconds. In addition the following changes were done. a) Enhance the stringpool to contain > 4Gi items ("stp_array_size" variable) b) stp_expand_array.c : Expand the "stp_array" array (that holds the items for garbage collection) exponentially instead of linearly. c) lv_getslot.c : And handle an edge case (numElems == MAXINT4) introduced in a prior commit for YottaDB#80
estess
suggested changes
Nov 10, 2017
sr_port/lv_getslot.c
Outdated
| @@ -141,7 +141,7 @@ lvTreeNode *lvtreenode_getslot(symval *sym) | |||
| numElems = p->numAlloc; | |||
| else | |||
| numElems = LV_NEWBLOCK_INIT_ALLOC; | |||
| lvtreenode_newblock(sym, (numElems > MAXINT4) ? MAXINT4 : (numElems * 2)); | |||
| lvtreenode_newblock(sym, (numElems < MAXINT4) ? (numElems * 2) : MAXINT4); | |||
There was a problem hiding this comment.
This seems to have an edge case problem. As soon as numElems is > (MAXINT4 / 2) but is < MAXINT4, this expression is either going to pass a negative value to lvtreenode_newblock() or a wrapped (too small) positive value. Not sure exactly which but think it is the former because numElems is unsigned which I think forces unsigned arithmetic. Yet the second parm to lvtreenode_newblock() is defined as being an int so I'm puzzled by the lack of a type sign conflict warning. Were the parm unsigned, there would still be an issue because again, values of numElems where (MAXINT4 / 2) < numElems < MAXINT4, the value of that expression will be larger than when numElems >= MAXINT4.
There was a problem hiding this comment.
Thanks for the catch. Now fixed.
sr_port/stp_expand_array.c
Outdated
| a = stp_array; | ||
| stp_array = (mstr **) malloc(stp_array_size * SIZEOF(mstr *)); | ||
| stp_array = (mstr **)malloc(stp_array_size * SIZEOF(mstr *)); | ||
| longcpy((uchar_ptr_t)stp_array, (uchar_ptr_t)a, n * SIZEOF(mstr *)); |
There was a problem hiding this comment.
Opportunity for longcopy->memcpy (optional)
There was a problem hiding this comment.
Done here and all other remaining usages.
…in lv_getslot YottaDB#80 These changes address review comments 1) All longcpy usages need to be replaced with memcpy. Since there were only a few remaining in the codebase, I replaced all of them. 2) lv_getslot.c : In function lvtreenode_getslot(), as soon as numElems is > (MAXINT4 / 2) but is < MAXINT4, we would end up passing a number that is > MAXINT4 but < MAXUINT4 to lvtreenode_newblock(). The latter expects a signed int though and so is going to see this as a negative number. Fix it so we never pass more than MAXINT4 to lvtreenode_newblock(). While in this module, fix a few assignments to be DEBUG_ONLY and move them to their proper scope and add a few missing asserts in lv_getslot() similar to the other two (lvtree_getslot() and lvtreenode_getslot()).
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.
…pool garbage collection in YottaDB) by choosing a better pivot #85
The primary enhancement is to stpg_sort.c. Previously, the pivot was chosen as the median of the left, right and (left+right)/2 indices in the array. This is the normally recommended pivot rule for quicksort. But it was observed to give us a pivot that ended up almost in one corner of the array (i.e. too close to left or too close to right). This in turn caused quicksort to degenerate into an O(n^2) algorithm which showed its colors when a garbage collection had to run with thousands of items. The pivot is now chosen as the median of 9 equally-spaced numbers in the array spanning [left,right] indices. And this was observed to give us a pivot that ended up almost in the midpoint of the array (45% most of the time) thus enabling quicksort to run in O(nlogn). With these changes, a garbage collection that used to take 83 seconds took 0.5 seconds.
In addition the following changes were done.
a) Enhance the stringpool to contain > 4Gi items ("stp_array_size" variable)
b) stp_expand_array.c : Expand the "stp_array" array (that holds the items for garbage collection) exponentially instead of linearly.
c) lv_getslot.c : And handle an edge case (numElems == MAXINT4) introduced in a prior commit for #80