Speed up cluster merge by batch copying arrays #280
Merged
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
christian-rauch
requested changes
Jun 15, 2023
There was a problem hiding this comment.
Thanks for this impressive speedup. I guess the improvement comes from changing the loop over source with element-wise calls to zarray_add, to the single memcpy?
I think the commit could use an explanation of what is going on and where the speed improvements come from.
common/zarray.h
Outdated
| @@ -437,26 +437,36 @@ static inline int zarray_index_of(const zarray_t *za, const void *p) | |||
| return -1; | |||
| } | |||
|
|
|||
| /** | |||
| * Add elements from start up to and excluding end from 'source' into 'dest'. | |||
| * el_size must be the same for both lists | |||
There was a problem hiding this comment.
Should "el_size" here be el_sz instead?
common/zarray.h
Outdated
Comment on lines
467
to
470
| static inline void zarray_add_all(zarray_t * dest, const zarray_t * source) | ||
| { | ||
| assert(dest->el_sz == source->el_sz); | ||
|
|
||
| // Don't allocate on stack because el_sz could be larger than ~8 MB | ||
| // stack size | ||
| char *tmp = (char*)calloc(1, dest->el_sz); | ||
|
|
||
| for (int i = 0; i < zarray_size(source); i++) { | ||
| zarray_get(source, i, tmp); | ||
| zarray_add(dest, tmp); | ||
| } | ||
|
|
||
| free(tmp); | ||
| zarray_add_range(dest, source, 0, source->size); | ||
| } |
There was a problem hiding this comment.
Does it make sense to keep the function zarray_add_all if it only maps to zarray_add_range? We could also just search & replace all calls to zarray_add_all instead.
merge_clusters was using zarray_add_all which was copying over elements one-by-one doing 2 memcpys and a potential array resize per element. Here we replace it by a range copy that does a single resize and memcpy for the operation which is a lot faster. In my testing it reduces the total runtime for an image that's 2000x3000 by 20%.
bouk
force-pushed
the
bouk/faster-cluster-merge
branch
from
0ec3dbb
to
e1b143c
Compare
|
@christian-rauch addressed your comments! |
christian-rauch
approved these changes
Jun 16, 2023
This was referenced Aug 4, 2023
Closed
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.
I was looking through the code and found that a lot of time was being spent on 'make clusters' so I did a quick look through why that is the case. I found the merge_clusters function to be inefficient, so I optimized it to do batch memcopies wherever possible.
This gives me a ~20% reduction in time spent for a large image.
Before
After