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

faster prod(::Array{BigInt}) #41014

Merged
merged 2 commits into from
Jun 10, 2021
Merged

faster prod(::Array{BigInt}) #41014

merged 2 commits into from
Jun 10, 2021

Conversation

rfourquet
Copy link
Member

By having a first pass on the array to compute the size of the result
(this avoids re-allocations).

Example:

julia> xs = rand(1:big(2)^20, 20);

julia> @btime prod($xs); # master
  2.050 μs (57 allocations: 1.23 KiB)

julia> @btime prod($xs); # PR
  310.413 ns (2 allocations: 192 bytes)

@rfourquet rfourquet added performance Must go faster domain:bignums BigInt and BigFloat labels May 30, 2021
Copy link
Member

@oscardssmith oscardssmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

base/gmp.jl Outdated
Comment on lines 640 to 642
nlimbs = sum(arr; init=0) do x
abs(x.size)
end
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd worry slightly this has exponential memory blowup. I realize that this is already the case for multiplication generally, but if most of arr was ones or other small numbers, this seems like it would vastly over-estimate the size of the final result. Then if prod is repeated, would it double the size of the allocation again? I notice that we don't estimate the size of mul normally.

The old comment also claimed this performs the operations in a less efficient order? The benchmark you gave might be multiplying relatively few, small numbers (20) to see the advantage?

Copy link
Member Author

@rfourquet rfourquet May 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can experiment with making an estimate at the bit level instead of the limb level, this would limit the blowup.

Then if prod is repeated, would it double the size of the allocation again?

No, as this uses x.size which is exact, and can be less than the memory which was allocated (x.alloc).

The old comment also claimed this performs the operations in a less efficient order?

The old comment (by me), didn't take into account the possibility of pre-allocating, and underestimated the cost of allocations compared to that of the multiplication. I would tend to believe that the different order was more efficient (assuming equally sized integers) at least partly because there would be less intermediate allocations.

@rfourquet
Copy link
Member Author

Good to go?

@oscardssmith
Copy link
Member

It needs a rebate, but otherwise looks good.

@oscardssmith oscardssmith added the status:merge me PR is reviewed. Merge when all tests are passing label Jun 8, 2021
@DilumAluthge DilumAluthge reopened this Jun 9, 2021
@rfourquet rfourquet closed this Jun 9, 2021
@rfourquet rfourquet reopened this Jun 9, 2021
@DilumAluthge
Copy link
Member

@rfourquet Can you try rebasing again on the latest master? These test failures have been fixed on master.

By having a first pass on the array to compute the size of the result
(this avoids re-allocations).

Example:
```julia
julia> xs = rand(1:big(2)^20, 20);

julia> @Btime prod($xs); # master
  2.050 μs (57 allocations: 1.23 KiB)

julia> @Btime prod($xs); # PR
  310.413 ns (2 allocations: 192 bytes)
```
@oscardssmith oscardssmith merged commit 8f57f88 into master Jun 10, 2021
@oscardssmith oscardssmith deleted the rf/bigint-prod branch June 10, 2021 16:52
@DilumAluthge DilumAluthge removed the status:merge me PR is reviewed. Merge when all tests are passing label Jun 18, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Use a first pass on the array to compute the size of the result.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:bignums BigInt and BigFloat performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants