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 indexing with UInt for Arrays #29784

Merged
merged 1 commit into from
Oct 25, 2018
Merged

Conversation

KristofferC
Copy link
Sponsor Member

Potential idea for fixing #17103 (at least for UInt).

Not sure if it is worth it though.

@mbauman, thoughts?

@KristofferC KristofferC added performance Must go faster domain:arrays [a, r, r, a, y, s] labels Oct 23, 2018
@mbauman
Copy link
Sponsor Member

mbauman commented Oct 23, 2018

So in the error case this would trade an InexactError with the value provided for a bounds error with a strange negative number. Have you measured how big of a performance improvement we get in exchange?

@KristofferC
Copy link
Sponsor Member Author

Yes, that is the trade-off.

Regarding performance

function mysum_uint(data)
    sum = zero(eltype(data))
    for i in UInt(1):UInt(lastindex(data))
        @inbounds sum += data[i]
    end
    return sum
end

using BenchmarkTools

siz = 128 * 2^20;
data = zeros(UInt8, siz);
@btime mysum_uint(data);

gives on master

julia> @btime mysum_uint(data);
  59.974 ms (0 allocations: 0 bytes)

but with the PR

julia> @btime mysum_uint(data)
 7.860 ms (0 allocations: 0 bytes)

it is, of course, the extreme case.

Copy link
Sponsor Member

@mbauman mbauman left a comment

Choose a reason for hiding this comment

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

Seems worthwhile.

@fredrikekre fredrikekre merged commit 2f27db9 into master Oct 25, 2018
@fredrikekre fredrikekre deleted the kc/fix_uint_slowdown branch October 25, 2018 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants