Skip to content

Improve performance and efficiency of Array.Parallel.choose#58

Closed
jack-pappas wants to merge 1 commit intodotnet:fsharp4from
jack-pappas:opt-array-parallel
Closed

Improve performance and efficiency of Array.Parallel.choose#58
jack-pappas wants to merge 1 commit intodotnet:fsharp4from
jack-pappas:opt-array-parallel

Conversation

@jack-pappas
Copy link
Copy Markdown
Contributor

I've re-implemented the Array.Parallel.choose function so it's more memory-efficient for typical use cases. The current implementation allocates two arrays (of 'U and bool, respectively) of the same length as the input array to hold intermediate results; this wastes a fair bit of memory when the input array is large, and especially when the proportion of 'chosen' elements is small.

The downside is that the new code is somewhat more complex than the existing implementation. I've introduced additional comparer types and the private combineWorkerResults function to merge intermediate results into the final result; this infrastructure can be re-used in the future if we want to add additional functions from the Array module to the Array.Parallel module (e.g., filter, choose2) or retrofit existing functions like Array.Parallel.partition for more efficiency/performance.

@dsyme
Copy link
Copy Markdown
Contributor

dsyme commented Jan 19, 2015

Hi @jack-pappas - Cool, do you have perf results? Also, are new tests needed?

@jack-pappas
Copy link
Copy Markdown
Contributor Author

@dsyme I don't have any perf. results, beyond that the new function "seems" to be at least as fast, if not faster. The reduction in memory usage was the main impetus behind this new implementation. I had some cases where I was using Array.Parallel.choose on large arrays of small data types (byte, char, etc.) and choosing only small fractions of the input data, and I realized the existing function was using O(input) memory instead of O(output) memory.

I can put some simple benchmarks together though to compare the performance of the two implementations if you'd like.

I don't think any new tests are needed -- the unit test suite already has tests for Array.Parallel.choose, and those should sufficiently cover this new implementation as well. Although not part of this official repository, I'm using this same implementation in a number of other Array.Parallel functions in ExtCore which have passing unit tests. (I meant to submit them for F# 4.0 and didn't have the time over the holidays, and now we've reached the feature freeze.)

@dsyme
Copy link
Copy Markdown
Contributor

dsyme commented Jan 19, 2015

@latkin - As per my other messages, we should draft guidelines on what's needed for a PR for library function perf improvements.

@jack-pappas - I think we can assume that the guidelines will say "Perf results must be submitted" :)

@latkin - perhaps we should start a branch for post-F#-4.0 PRs?

@latkin
Copy link
Copy Markdown
Contributor

latkin commented Jan 23, 2015

Thank you for the submission. Per the contribution guidelines, perf optimizations should include tests and quantitative analysis showing reliable gains from the change, which others can reproduce. Please re-open when tests and analysis are in place.

@latkin latkin closed this Jan 23, 2015
@dsyme
Copy link
Copy Markdown
Contributor

dsyme commented Jan 26, 2015

I tweeted a request that people add the required perf/correctness testing and resubmit https://twitter.com/dsyme/status/559725937940393988

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants