Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fix in parallel_scan #60

Merged
merged 4 commits into from
Dec 22, 2021
Merged

Bug fix in parallel_scan #60

merged 4 commits into from
Dec 22, 2021

Conversation

Sudha247
Copy link
Contributor

Fixes bugs reported in #38 and #59. The fixes are:

  1. Wrong entry in result: The final entry in the array was wrong when ~num_additional_domains:1. Caused by pool size 2 being omitted while accounting for offsets, fixed in e03ed86

  2. Rejecting array size less than pool size: add a fast path in parallel scan #39 also tries to fix this by calling a sequential scan when pool size is greater than length of the input array. As @nilsbecker pointed out, this may not be the most efficient way for long running operations. What's done here is instead assuming the scan is going to be run on n domains, where n is the size of the array. So, the pool size is minimum of number of domains available and length of array. So each operation runs on a domain of its own when number of domains is greater than array length. (fixed in c8bdc24).

Also adds @jmid's test in #59.

lib/task.ml Outdated Show resolved Hide resolved
@kayceesrk
Copy link
Contributor

@jmid If this looks good to you, let me know (or mark as approved) and I can merge this one..

@jmid
Copy link
Contributor

jmid commented Dec 22, 2021

LGTM! 👍‍
I've also run a few local tests on 4.12.0+domains and a recent 4.14.0+domains with these on top of #50 and it works nicely.
Thanks @Sudha247!

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