SP: optimize using minmax_element 25% faster #1326
Conversation
than using both min() and max() separately
|
@mrcslws Please see here |
| @@ -978,11 +976,11 @@ Real SpatialPooler::avgConnectedSpanForColumn2D_(UInt column) | |||
| return 0; | |||
| } | |||
|
|
|||
| UInt rowSpan = *max_element(rows.begin(),rows.end()) - | |||
| *min_element(rows.begin(),rows.end()) + 1; | |||
| auto minmaxRows = minmax_element(rows.begin(),rows.end()); | |||
There was a problem hiding this comment.
Whitespace cleanup: put a space after the comma.
| UInt rowSpan = *max_element(rows.begin(),rows.end()) - | ||
| *min_element(rows.begin(),rows.end()) + 1; | ||
| auto minmaxRows = minmax_element(rows.begin(),rows.end()); | ||
| UInt rowSpan = *minmaxRows.second /*max*/ - *minmaxRows.first /*min*/ +1; |
There was a problem hiding this comment.
Whitespace cleanup: put a space after the +.
|
|
||
| UInt colSpan = *max_element(cols.begin(),cols.end()) - | ||
| *min_element(cols.begin(),cols.end()) + 1; | ||
| auto minmaxCols = minmax_element(cols.begin(),cols.end()); |
There was a problem hiding this comment.
Whitespace cleanup: put a space after the comma.
| UInt colSpan = *max_element(cols.begin(),cols.end()) - | ||
| *min_element(cols.begin(),cols.end()) + 1; | ||
| auto minmaxCols = minmax_element(cols.begin(),cols.end()); | ||
| UInt colSpan = *minmaxCols.second - *minmaxCols.first +1; |
There was a problem hiding this comment.
Whitespace cleanup: put a space after the +.
|
Why do you say this is 25% faster? Did you run a quick microbenchmark on
these two approaches?
Not the whole sp, but this function is.
Before it was 2x O(n), now 1x O(3/2n), I think I mentioned it in the commit
description.
|
|
You can't really assign a number to this improvement a priori. It's doing 1 pass instead of 2, which is good, but it's hard to predict how much faster it actually is. Doing 2 passes isn't as slow as you might think, since the CPU prefetches memory during sequential lookups. (I don't see this in any of your commit descriptions. I checked before saying anything.) Anyway, this is still a good change. |
My bad, sorry. I thought I've documented that somewhere. Should I try some micro-benchmark, or is it ok as is? (except the whitespaces..) |
|
This is good as-is, just fix the whitespace and I'll merge it. |
|
@mrcslws Sorry for the long delay, I'm back home. Fixed the cleanup and should be ready. |
|
👏 |
than using both min() and max() separately
Fixes #1325