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

Delete usage of inference in SubArray code #12409

Closed
wants to merge 3 commits into from
Closed

Conversation

timholy
Copy link
Member

@timholy timholy commented Jul 31, 2015

It was recently noticed that the subarray code called Base.return_types from a @generated function, and that this is a no-no. This PR eliminates that call.

There are a few interesting things to discuss here:

  • For reasons I haven't yet pinned down, inference and dispatch seem to fail on merge_indexes. I suspect it has to do with the constructs like V.indexes[2:end]. As a bandaid, I added a type annotation to its output.
  • The first commit, adding @inferrable to the sub and slice tests, seems to work fine in our standard tests. However, if you run with JULIA_TESTFULL it seems to be an effective way of uncovering some lurking bug:
$ JULIA_TESTFULL=1 ./julia
               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "help()" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.4.0-dev+6409 (2015-07-30 15:23 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit eb2f341* (0 days old master)
|__/                   |  x86_64-linux-gnu

shell> cd test
/home/tim/src/julia/test

julia> using Base.Test

julia> include("subarray.jl")  # now wait a LOONG time
typeof(A) = SubArray{Int64,3,Array{Int64,3},Tuple{Array{Int64,1},StepRange{Int64,Int64},Array{Int64,1}},0}
A.indexes = ([8,4,6,12,5,7],13:-2:1,[8,4,6,12,5,7])
I = (2:5,1:2:5,1:2:5)
ERROR: LoadError: SYSTEM: show(lasterr) caused an error

EDIT: I should add that if you try that supposedly-failing test directly, it works fine:

julia> B = reshape(1:13^3, 13, 13, 13);

julia> A = sub(B, [8,4,6,12,5,7], 13:-2:1, [8,4,6,12,5,7]);

julia> S = sub(A, 2:5, 1:2:5, 1:2:5)
4x3x3 SubArray{Int64,3,Array{Int64,3},Tuple{Array{Int64,1},StepRange{Int64,Int64},Array{Int64,1}},0}:
[:, :, 1] =
 1343  1291  1239
 1345  1293  1241
 1351  1299  1247
 1344  1292  1240

[:, :, 2] =
 1005  953  901
 1007  955  903
 1013  961  909
 1006  954  902

[:, :, 3] =
 836  784  732
 838  786  734
 844  792  740
 837  785  733

julia> S == A[2:5,1:2:5, 1:2:5]
true

Because it's history-dependent, it suggests a "deep" bug.

Not sure what to do about that. I could test what happens if we drop the first commit and just do the second. It would be nice, of course, to ensure that these operations remain inferrable.

CC @carnaval, @jakebolewski, @JeffBezanson.

@simonster
Copy link
Member

You could replace the tuple indexing with tail(x) (actually need IVindex nested calls) to get type information and avoid heap-allocating the new tuples. Not sure if it would help with the issue.

@mbauman
Copy link
Member

mbauman commented Jul 31, 2015

I was toying with this a bit last night, too. I came to a nearly identical solution, but it doesn't quite taste right to me. It'd be nice to allow inference to do this properly through a function call boundary. That is, sub and slice would simply compute the new indices (without worrying about their types), and then punt to an outer SubArray constructor to do the rest:

@generated function SubArray{T,N}(A::AbstractArray{T}, I::Tuple{Vararg{ViewIndex}}, dims::NTuple{N,Int})
    Ip = I.parameters
    LD = subarray_linearindexing_dim(A, I)
    strideexpr = stride1expr(A, Ip, :A, :I, LD)
    exfirst = first_index_expr(:A, :I, length(Ip))
    quote
        $exfirst
        SubArray{$T,$N,$A,$I,$LD}(A, I, dims, f, $strideexpr)
    end
end

But of course, that doesn't work. There's more inter-woven logic thoughout sub and slice (particularly for nested cases) that's required to do this right. It'd really be nice to split that apart, but I haven't spent the time required to understand the interplay between I (the computed indices) and J (the passed indices) well enough to tackle such a refactoring on my own.

@mbauman
Copy link
Member

mbauman commented Jul 31, 2015

In any case, 👍 on this solution.

@timholy
Copy link
Member Author

timholy commented Jul 31, 2015

The comments from @simonster and @mbauman combine with thoughts I've had, wondering if we could rewrite the SubArray code in a more Lispy manner and recurse over the supplied indexes. One of the key reasons the old SubArrays had such poor construction performance was because of type-instability: looping over containers (the indexes tuple) where each element might have a different type makes things slow. The new SubArrays get around that using @generated functions, but it occurs that one might be able to do it by "peeling off" one index at a time, by means similar to those discussed in #10337 (and somewhere else I can't find right now). For performance reasons we ultimately had to abandon that effort and use @generated functions, but there's hope of getting back to such ideas.

It would be vastly more complicated to pull this off for SubArrays, but it may merit some serious thought. However, that's not something to tackle now; it's a better project for 0.5.

@timholy
Copy link
Member Author

timholy commented Jul 31, 2015

I would be interested in learning whether anyone else can reproduce the weird JULIA_TESTFULL problem on this branch.

@timholy
Copy link
Member Author

timholy commented Jul 31, 2015

@simonster, I tested nested-tail, but it didn't fix anything.

@timholy
Copy link
Member Author

timholy commented Jan 2, 2016

Just tried this against current master. The mysterious codegen failure with JULIA_TESTFULL is now gone.

In the absence of further commentary, I will probably wait until #14529 gets merged (that PR adds new tests), rebase this and run it the TESTFULL suite again (which takes many hours), and then merge this.

@timholy timholy force-pushed the teh/sub_inference branch from 4c482a0 to c104084 Compare January 3, 2016 13:23
@timholy
Copy link
Member Author

timholy commented Jan 3, 2016

Hmm, bad news: this nearly doubles the time needed for the subarray tests, which are already the slowest tests in the entire suite. I don't think we can merge this unless we trim down the tests or speed up codegen. (It's all codegen: while the first run is many minutes, a second run without recompilation is about 1s.)

@mbauman
Copy link
Member

mbauman commented Feb 17, 2016

Fixed by #15071

@mbauman mbauman closed this Feb 17, 2016
@DilumAluthge DilumAluthge deleted the teh/sub_inference branch March 25, 2021 22:11
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