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

Ensure SubArray's unsafe_getindex doesn't check bounds #11625

Merged
merged 1 commit into from
Jun 9, 2015
Merged

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Jun 9, 2015

This resolves the core issue behind the regressions in SubArray's performance as reported in #5117. Unfortunately, without #7799, the surface syntax isn't very nice since it's a ton of unsafe_ function calls.

Using the test case from #5117 (comment) :

  • Julia pre-10525 (benchmark as written, with @inbounds)
follow up: mykern( 2:(siz - 3), s, a, x, y )
   9.484 milliseconds (157 allocations: 10893 bytes)
follow up: mykern( 2:(siz - 3), s1, a, x1, y1 )
  68.632 milliseconds (16353 allocations: 718 KB)
follow up: mykern( 3:(siz - 2), s2, a, x2, y2 )
  16.753 milliseconds (6 allocations: 192 bytes)
  15.493 milliseconds (6 allocations: 192 bytes)
  17.036 milliseconds (6 allocations: 192 bytes)
  • This branch (benchmark rewritten to use unsafe_getindex/unsafe_setindex!):
follow up: mykern( 2:(siz - 3), s, a, x, y )
  10.046 milliseconds (158 allocations: 11469 bytes)
follow up: mykern( 2:(siz - 3), s1, a, x1, y1 )
  70.425 milliseconds (19097 allocations: 827 KB)
follow up: mykern( 3:(siz - 2), s2, a, x2, y2 )
  14.263 milliseconds (6 allocations: 192 bytes)
  16.470 milliseconds (6 allocations: 192 bytes)
  16.711 milliseconds (6 allocations: 192 bytes)

This resolves the core issue behind SubArray's performance as reported in #5117.  Unfortunately, without #7799, the surface syntax isn't very nice since it's a ton of unsafe_ function calls.

Using the test case from #5117 (comment) :

* Julia pre-10525 (with `@inbounds`)

```
follow up: mykern( 2:(siz - 3), s, a, x, y )
   9.484 milliseconds (157 allocations: 10893 bytes)
follow up: mykern( 2:(siz - 3), s1, a, x1, y1 )
  68.632 milliseconds (16353 allocations: 718 KB)
follow up: mykern( 3:(siz - 2), s2, a, x2, y2 )
  16.753 milliseconds (6 allocations: 192 bytes)
  15.493 milliseconds (6 allocations: 192 bytes)
  17.036 milliseconds (6 allocations: 192 bytes)
```

* This branch (with `unsafe_getindex`/`unsafe_setindex!`):

```
follow up: mykern( 2:(siz - 3), s, a, x, y )
  10.046 milliseconds (158 allocations: 11469 bytes)
follow up: mykern( 2:(siz - 3), s1, a, x1, y1 )
  70.425 milliseconds (19097 allocations: 827 KB)
follow up: mykern( 3:(siz - 2), s2, a, x2, y2 )
  14.263 milliseconds (6 allocations: 192 bytes)
  16.470 milliseconds (6 allocations: 192 bytes)
  16.711 milliseconds (6 allocations: 192 bytes)
```
@mbauman mbauman mentioned this pull request Jun 9, 2015
@mbauman
Copy link
Member Author

mbauman commented Jun 9, 2015

JULIA_TESTFULL for subarray passes without a hitch, as well.

@timholy
Copy link
Member

timholy commented Jun 9, 2015

Nice find! Clear evidence that skipping bounds checks basically needs to propagate downwards.

Just to check, are you saying that to get this performance you need to modify the mykern script to explicitly use unsafe_getindex(s, i) rather than s[i]?

@mbauman
Copy link
Member Author

mbauman commented Jun 9, 2015

Just to check, are you saying that to get this performance you need to modify the mykern script to explicitly use unsafe_getindex(s, i) rather than s[i]?

Yes, exactly.

@timholy
Copy link
Member

timholy commented Jun 9, 2015

Very good. Clearly this is a nice step forward.

Can you also ("temporarily") turn off the explicit bounds checking for getindex? Let the underlying array still check bounds, of course. I think changing all s[i] to unsafe_getindex(s, i) is just too ugly for user code.

@mbauman
Copy link
Member Author

mbauman commented Jun 9, 2015

What, you don't think it's reasonable to ask people to rewrite:

@inbounds s[i] = a * (x[i+1] - x[i]) / (y[i+1] - y[i])

to

unsafe_setindex!(s, a * (unsafe_getindex(x, i+1) - unsafe_getindex(x, i)) / (unsafe_getindex(y, i+1) - unsafe_getindex(y, i)), i)

for version 0.4? And then back again later? The latter is so much clearer and more explicit! ;-)

I'll merge this now and tackle the change in semantics separately, with the hope that it'll be reverted someday.

mbauman added a commit that referenced this pull request Jun 9, 2015
Ensure SubArray's unsafe_getindex doesn't check bounds
@mbauman mbauman merged commit c69cae3 into master Jun 9, 2015
@mbauman mbauman deleted the mb/unsafersub branch June 9, 2015 20:16
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.

2 participants